| Message ID | 20241203164143.29852-2-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: omap: allow building the module with COMPILE_TEST=y | expand |
Hi deeeee Ho peeps! On 03/12/2024 18:41, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We can drop the else branch if we get the clock already prepared using > the relevant helper. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- Booting a beaglebone black with the linux-next from Today fails (next-20241213). Enabling earlycon + debug yields below splat to be printed to the console: [ 2.628019] ------------[ cut here ]------------ [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254 clk_core_enable+0xb4/0x1b0 [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18 [ 2.646530] Modules linked in: [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15 [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree) [ 2.666531] Workqueue: events_unbound deferred_probe_work_func [ 2.672553] Call trace: [ 2.672570] unwind_backtrace from show_stack+0x10/0x14 [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64 [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130 [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0 [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0 [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c [ 2.708159] clk_core_enable_lock from sysc_enable_opt_clocks.part.9+0x28/0x84 [ 2.715611] sysc_enable_opt_clocks.part.9 from sysc_enable_module+0x254/0x2dc [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0 [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130 [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54 [ 2.741096] rpm_callback from rpm_resume+0x614/0x660 [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64 [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188 [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c or_thread from kthread+0x188/0x24c [ 2.789476] kthread from ret_from_fork+0x14/0x20 [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8) [ 2.799528] 1fa0: 00000000 00000000 00000000 00000000 [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 2.823173] ---[ end trace 0000000000000000 ]--- [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed for enable: -108 [ 2.835998] ------------[ cut here ]------------ reverting b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()") fixes the boot for me. > drivers/gpio/gpio-omap.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 54c4bfdccf568..57d299d5d0b16 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) > } > > if (bank->dbck_flag) { > - bank->dbck = devm_clk_get(dev, "dbclk"); > + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); > if (IS_ERR(bank->dbck)) { > dev_err(dev, > "Could not get gpio dbck. Disable debounce\n"); > bank->dbck_flag = false; > - } else { > - clk_prepare(bank->dbck); > } > } > I can only spot a minor functional change. The code prior this commit does not check the result of clk_prepare(), and does neither set bank->dbck_flag = false; nor call clk_put(); Other than that, timing is likely to be changed. Not sure what is the thing here. Yours, -- Matti
Hey Matti, how are you? On Fri, 2024-12-13 at 14:16 +0200, Matti Vaittinen wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > We can drop the else branch if we get the clock already prepared using > > the relevant helper. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > Booting a beaglebone black with the linux-next from Today fails > (next-20241213). Enabling earlycon + debug yields below splat to be > printed to the console: > > [ 2.628019] ------------[ cut here ]------------ > [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254 > clk_core_enable+0xb4/0x1b0 > [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18 > [ 2.646530] Modules linked in: > [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted > 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15 > [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree) > [ 2.666531] Workqueue: events_unbound deferred_probe_work_func > [ 2.672553] Call trace: > [ 2.672570] unwind_backtrace from show_stack+0x10/0x14 > [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64 > [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130 > [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0 > [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0 > [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c > [ 2.708159] clk_core_enable_lock from > sysc_enable_opt_clocks.part.9+0x28/0x84 > [ 2.715611] sysc_enable_opt_clocks.part.9 from > sysc_enable_module+0x254/0x2dc > [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0 > [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130 > [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54 > [ 2.741096] rpm_callback from rpm_resume+0x614/0x660 > [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64 > [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188 > [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c > or_thread from kthread+0x188/0x24c > [ 2.789476] kthread from ret_from_fork+0x14/0x20 > [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8) > [ 2.799528] 1fa0: 00000000 > 00000000 00000000 00000000 > [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000 > 00000000 00000000 00000000 > [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > [ 2.823173] ---[ end trace 0000000000000000 ]--- > [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed > for enable: -108 > [ 2.835998] ------------[ cut here ]------------ > > reverting > b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()") > > fixes the boot for me. > > > > drivers/gpio/gpio-omap.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 54c4bfdccf568..57d299d5d0b16 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) > > } > > > > if (bank->dbck_flag) { > > - bank->dbck = devm_clk_get(dev, "dbclk"); > > + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); > > if (IS_ERR(bank->dbck)) { > > dev_err(dev, > > "Could not get gpio dbck. Disable debounce\n"); > > bank->dbck_flag = false; > > - } else { > > - clk_prepare(bank->dbck); > > } > > } > > > > I can only spot a minor functional change. The code prior this commit > does not check the result of clk_prepare(), and does neither set > bank->dbck_flag = false; nor call clk_put(); > > Other than that, timing is likely to be changed. Not sure what is the > thing here. The new code looks more correct, with the return code check from clk_prepare(). Could it be that two problems eliminated themselves in your case before? ;-) Would it be possible for you to provide the logs with "initcall_debug" with and without the patch in question?
On Fri, Dec 13, 2024 at 1:29 PM Sverdlin, Alexander <alexander.sverdlin@siemens.com> wrote: > > Hey Matti, how are you? > > On Fri, 2024-12-13 at 14:16 +0200, Matti Vaittinen wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > We can drop the else branch if we get the clock already prepared using > > > the relevant helper. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > > Booting a beaglebone black with the linux-next from Today fails > > (next-20241213). Enabling earlycon + debug yields below splat to be > > printed to the console: > > > > [ 2.628019] ------------[ cut here ]------------ > > [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254 > > clk_core_enable+0xb4/0x1b0 > > [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18 > > [ 2.646530] Modules linked in: > > [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted > > 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15 > > [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree) > > [ 2.666531] Workqueue: events_unbound deferred_probe_work_func > > [ 2.672553] Call trace: > > [ 2.672570] unwind_backtrace from show_stack+0x10/0x14 > > [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64 > > [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130 > > [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0 > > [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0 > > [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c > > [ 2.708159] clk_core_enable_lock from > > sysc_enable_opt_clocks.part.9+0x28/0x84 > > [ 2.715611] sysc_enable_opt_clocks.part.9 from > > sysc_enable_module+0x254/0x2dc > > [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0 > > [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130 > > [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54 > > [ 2.741096] rpm_callback from rpm_resume+0x614/0x660 > > [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64 > > [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188 > > [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c > > or_thread from kthread+0x188/0x24c > > [ 2.789476] kthread from ret_from_fork+0x14/0x20 > > [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8) > > [ 2.799528] 1fa0: 00000000 > > 00000000 00000000 00000000 > > [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000 > > 00000000 00000000 00000000 > > [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > [ 2.823173] ---[ end trace 0000000000000000 ]--- > > [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed > > for enable: -108 > > [ 2.835998] ------------[ cut here ]------------ > > > > reverting > > b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()") > > > > fixes the boot for me. > > > > > > > drivers/gpio/gpio-omap.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > > index 54c4bfdccf568..57d299d5d0b16 100644 > > > --- a/drivers/gpio/gpio-omap.c > > > +++ b/drivers/gpio/gpio-omap.c > > > @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) > > > } > > > > > > if (bank->dbck_flag) { > > > - bank->dbck = devm_clk_get(dev, "dbclk"); > > > + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); > > > if (IS_ERR(bank->dbck)) { > > > dev_err(dev, > > > "Could not get gpio dbck. Disable debounce\n"); > > > bank->dbck_flag = false; > > > - } else { > > > - clk_prepare(bank->dbck); > > > } > > > } > > > > > > > I can only spot a minor functional change. The code prior this commit > > does not check the result of clk_prepare(), and does neither set > > bank->dbck_flag = false; nor call clk_put(); > > > > Other than that, timing is likely to be changed. Not sure what is the > > thing here. > > The new code looks more correct, with the return code check from clk_prepare(). > Could it be that two problems eliminated themselves in your case before? ;-) > Would it be possible for you to provide the logs with "initcall_debug" with > and without the patch in question? > > -- > Alexander Sverdlin > Siemens AG > www.siemens.com This commit was the tip of my for-next branch so I just dropped it to fix next. Thanks for the heads-up. Bart
On 13/12/2024 14:29, Sverdlin, Alexander wrote: > Hey Matti, how are you? Hiya Alex! Fine fine, just feeling a bit (a lot) older :) I hope Siemens(?) has been treating You well! Good to see you're still actively working with upstream stuff! It'd be great to see you again, maybe in some conference ;) And just in case you see other good old Ulmians - say: "Hi dee Ho peeps!" - from me ;) > On Fri, 2024-12-13 at 14:16 +0200, Matti Vaittinen wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >>> We can drop the else branch if we get the clock already prepared using >>> the relevant helper. >>> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> --- >> >> Booting a beaglebone black with the linux-next from Today fails >> (next-20241213). Enabling earlycon + debug yields below splat to be >> printed to the console: >> >> [ 2.628019] ------------[ cut here ]------------ >> [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254 >> clk_core_enable+0xb4/0x1b0 >> [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18 >> [ 2.646530] Modules linked in: >> [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted >> 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15 >> [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree) >> [ 2.666531] Workqueue: events_unbound deferred_probe_work_func >> [ 2.672553] Call trace: >> [ 2.672570] unwind_backtrace from show_stack+0x10/0x14 >> [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64 >> [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130 >> [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0 >> [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0 >> [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c >> [ 2.708159] clk_core_enable_lock from >> sysc_enable_opt_clocks.part.9+0x28/0x84 >> [ 2.715611] sysc_enable_opt_clocks.part.9 from >> sysc_enable_module+0x254/0x2dc >> [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0 >> [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130 >> [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54 >> [ 2.741096] rpm_callback from rpm_resume+0x614/0x660 >> [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64 >> [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188 >> [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c >> or_thread from kthread+0x188/0x24c >> [ 2.789476] kthread from ret_from_fork+0x14/0x20 >> [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8) >> [ 2.799528] 1fa0: 00000000 >> 00000000 00000000 00000000 >> [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000 >> 00000000 00000000 00000000 >> [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 >> [ 2.823173] ---[ end trace 0000000000000000 ]--- >> [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed >> for enable: -108 >> [ 2.835998] ------------[ cut here ]------------ >> >> reverting >> b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()") >> >> fixes the boot for me. >> >> >>> drivers/gpio/gpio-omap.c | 4 +--- >>> 1 file changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >>> index 54c4bfdccf568..57d299d5d0b16 100644 >>> --- a/drivers/gpio/gpio-omap.c >>> +++ b/drivers/gpio/gpio-omap.c >>> @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) >>> } >>> >>> if (bank->dbck_flag) { >>> - bank->dbck = devm_clk_get(dev, "dbclk"); >>> + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); >>> if (IS_ERR(bank->dbck)) { >>> dev_err(dev, >>> "Could not get gpio dbck. Disable debounce\n"); >>> bank->dbck_flag = false; >>> - } else { >>> - clk_prepare(bank->dbck); >>> } >>> } >>> >> >> I can only spot a minor functional change. The code prior this commit >> does not check the result of clk_prepare(), and does neither set >> bank->dbck_flag = false; nor call clk_put(); >> >> Other than that, timing is likely to be changed. Not sure what is the >> thing here. > > The new code looks more correct, with the return code check from clk_prepare(). I agree. Unfortunately something breaks though. > Could it be that two problems eliminated themselves in your case before? ;-) Ha. Would it be the first time? ;) Well, unfortunately I really don't know the omap/am335x stuff too well. Maybe Tony (CC) or Grygori know better - I think they have helped me with BBB related stuff before :) > Would it be possible for you to provide the logs with "initcall_debug" with > and without the patch in question? Logs attached :) minicom_reverted.cap has is the next-20241213 having this commit reverted. minicom_not_reverted.cap is just the next-20241213. Unfortunately it seems there are some errors (flipped bits?) in the serial log. Not sure what causes them - I have new FTDI chip, perhaps it causes these problems. (Comparison of the logs shows single, seemingly random, letters being changed.) Yours, -- Matti
Hi Matti! On Fri, 2024-12-13 at 15:55 +0200, Matti Vaittinen wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > We can drop the else branch if we get the clock already prepared using > > > > the relevant helper. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > --- > > > > > > Booting a beaglebone black with the linux-next from Today fails > > > (next-20241213). Enabling earlycon + debug yields below splat to be > > > printed to the console: > > > > > > [ 2.628019] ------------[ cut here ]------------ > > > [ 2.632793] WARNING: CPU: 0 PID: 34 at drivers/clk/clk.c:1254 > > > clk_core_enable+0xb4/0x1b0 > > > [ 2.641156] Enabling unprepared l4-wkup-clkctrl:0008:18 > > > [ 2.646530] Modules linked in: > > > [ 2.649688] CPU: 0 UID: 0 PID: 34 Comm: kworker/u4:3 Not tainted > > > 6.13.0-rc2-next-20241213-00002-gf2d4b29c8330 #15 > > > [ 2.660256] Hardware name: Generic AM33XX (Flattened Device Tree) > > > [ 2.666531] Workqueue: events_unbound deferred_probe_work_func > > > [ 2.672553] Call trace: > > > [ 2.672570] unwind_backtrace from show_stack+0x10/0x14 > > > [ 2.680578] show_stack from dump_stack_lvl+0x50/0x64 > > > [ 7 2.685788] dump_stack_lvl from __warn+0xc0/0x130 > > > [ 2.690734] __warn from warn_slowpath_fmt+0x80/0x1a0 > > > [ 2.695944] warn_slowpath_fmt from clk_core_enable+0xb4/0x1b0 > > > [ 2.701963] clk_core_enable from clk_core_enable_lock+0x18/0x2c > > > [ 2.708159] clk_core_enable_lock from > > > sysc_enable_opt_clocks.part.9+0x28/0x84 > > > [ 2.715611] sysc_enable_opt_clocks.part.9 from > > > sysc_enable_module+0x254/0x2dc > > > [ 2.723052] sysc_enable_module from sysc_runtime_resume+0x17c/0x1c0 > > > [ 2.729599] sysc_runtime_resume from __rpm_callback+0x4c/0x130 > > > [ 2.735709] __rpm_callback from rpm_callback+0x50/0x54 > > > [ 2.741096] rpm_callback from rpm_resume+0x614/0x660 > > > [ 2.746304] rpm_resume from __pm_runtime_resume+0x4c/0x64 > > > [ 2.751960] __pm_runtime_resume from __device_attach+0xd0/0x188 > > > [ 2.758155] __device_attach from bus_probe_device+0x88/0x8c > > > or_thread from kthread+0x188/0x24c > > > [ 2.789476] kthread from ret_from_fork+0x14/0x20 > > > [ 2.794327] Exception stack(0xe0091fb0 to 0xe0091ff8) > > > [ 2.799528] 1fa0: 00000000 > > > 00000000 00000000 00000000 > > > [ 2.807947] 1fc0: 00000000 00000000 00000000 00000000 00000000 > > > 00000000 00000000 00000000 > > > [ 2.816365] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 > > > [ 2.823173] ---[ end trace 0000000000000000 ]--- > > > [ 2.828070] ti-sysc 44e07000.target-module: Optional clocks failed > > > for enable: -108 > > > [ 2.835998] ------------[ cut here ]------------ > > > > > > reverting > > > b7bbaff8c1bc ("gpio: omap: save two lines by using devm_clk_get_prepared()") > > > > > > fixes the boot for me. > > > > > > > > > > drivers/gpio/gpio-omap.c | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > > > index 54c4bfdccf568..57d299d5d0b16 100644 > > > > --- a/drivers/gpio/gpio-omap.c > > > > +++ b/drivers/gpio/gpio-omap.c > > > > @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) > > > > } > > > > > > > > if (bank->dbck_flag) { > > > > - bank->dbck = devm_clk_get(dev, "dbclk"); > > > > + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); > > > > if (IS_ERR(bank->dbck)) { > > > > dev_err(dev, > > > > "Could not get gpio dbck. Disable debounce\n"); > > > > bank->dbck_flag = false; > > > > - } else { > > > > - clk_prepare(bank->dbck); > > > > } > > > > } > > > > > > > > > > I can only spot a minor functional change. The code prior this commit > > > does not check the result of clk_prepare(), and does neither set > > > bank->dbck_flag = false; nor call clk_put(); > > > > > > Other than that, timing is likely to be changed. Not sure what is the > > > thing here. > > > > The new code looks more correct, with the return code check from clk_prepare(). > > I agree. Unfortunately something breaks though. > > > Could it be that two problems eliminated themselves in your case before? ;-) > > Ha. Would it be the first time? ;) Well, unfortunately I really don't > know the omap/am335x stuff too well. Maybe Tony (CC) or Grygori know > better - I think they have helped me with BBB related stuff before :) > > > Would it be possible for you to provide the logs with "initcall_debug" with > > and without the patch in question? > > Logs attached :) minicom_reverted.cap has is the next-20241213 having > this commit reverted. minicom_not_reverted.cap is just the next-20241213. > > Unfortunately it seems there are some errors (flipped bits?) in the > serial log. Not sure what causes them - I have new FTDI chip, perhaps it > causes these problems. (Comparison of the logs shows single, seemingly > random, letters being changed.) No problem! Thanks for the logs! I think I know what happened: I suppose it's "prepared" counter underflow on probe deferral of GPIO driver (there are "probe of 44e07000.gpio returned 517" visible). If you'd still have a chance to test 6.13.0-rc2-next-20241213, I believe this was missing in the "gpio: omap: save two lines by using devm_clk_get_prepared()": diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 76d5d87e9681..0c30013d2b48 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1473,8 +1473,6 @@ static int omap_gpio_probe(struct platform_device *pdev) if (ret) { pm_runtime_put_sync(dev); pm_runtime_disable(dev); - if (bank->dbck_flag) - clk_unprepare(bank->dbck); return ret; } @@ -1495,8 +1493,6 @@ static void omap_gpio_remove(struct platform_device *pdev) cpu_pm_unregister_notifier(&bank->nb); gpiochip_remove(&bank->chip); pm_runtime_disable(&pdev->dev); - if (bank->dbck_flag) - clk_unprepare(bank->dbck); } static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev)
On 16/12/2024 10:57, Sverdlin, Alexander wrote: > Hi Matti! > > On Fri, 2024-12-13 at 15:55 +0200, Matti Vaittinen wrote: > >>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>>> >>>>> We can drop the else branch if we get the clock already prepared using >>>>> the relevant helper. >>>>> >>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>>> --- >>>> >>>> Booting a beaglebone black with the linux-next from Today fails >>>> (next-20241213). Enabling earlycon + debug yields below splat to be >>>> printed to the console: >>>> > > No problem! Thanks for the logs! I think I know what happened: I suppose > it's "prepared" counter underflow on probe deferral of GPIO driver > (there are "probe of 44e07000.gpio returned 517" visible). Ah. Indeed. The deferral is visible in the logs. > > If you'd still have a chance to test 6.13.0-rc2-next-20241213, > I believe this was missing in the > "gpio: omap: save two lines by using devm_clk_get_prepared()": > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 76d5d87e9681..0c30013d2b48 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1473,8 +1473,6 @@ static int omap_gpio_probe(struct platform_device *pdev) > if (ret) { > pm_runtime_put_sync(dev); > pm_runtime_disable(dev); > - if (bank->dbck_flag) > - clk_unprepare(bank->dbck); > return ret; > } > > @@ -1495,8 +1493,6 @@ static void omap_gpio_remove(struct platform_device *pdev) > cpu_pm_unregister_notifier(&bank->nb); > gpiochip_remove(&bank->chip); > pm_runtime_disable(&pdev->dev); > - if (bank->dbck_flag) > - clk_unprepare(bank->dbck); > } > > static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) > This fixes the boot as you assumed. I suppose this should be baked in the Bartosz's original patch assumed it was dropped from the GPIO tree. Furthermore, this seems to be a fix to a hidden problem on original code. If the original code failed in the clk_prepare() and then deferred probe(), this same problem should have appeared, right? Maybe consider using Fixes - tag even if this and the original change got squashed. Feel free to add a: Tested-By: Matti Vaittinen <mazziesaccount@gmail.com> if this fix goes to the tree "as is". Thanks for the fix! Yours, -- Matti.
On 16/12/2024 13:11, Matti Vaittinen wrote: > On 16/12/2024 10:57, Sverdlin, Alexander wrote: ... > This fixes the boot as you assumed. > I suppose this should be baked in the Bartosz's original patch assumed > it was dropped from the GPIO tree. > > Furthermore, this seems to be a fix to a hidden problem on original > code. Just a minor clarification: "Original code" here refers to the code before the "gpio: omap: save two lines by using devm_clk_get_prepared()" > If the original code failed in the clk_prepare() and then deferred > probe(), this same problem should have appeared, right? > > Maybe consider using Fixes - tag even if this and the original change > got squashed. Feel free to add a: Yours, -- Matti
Thanks for quick re-test Matti! On Mon, 2024-12-16 at 13:11 +0200, Matti Vaittinen wrote: > > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > > > > > We can drop the else branch if we get the clock already prepared using > > > > > > the relevant helper. > > > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > --- > > > > > > > > > > Booting a beaglebone black with the linux-next from Today fails > > > > > (next-20241213). Enabling earlycon + debug yields below splat to be > > > > > printed to the console: > > > > > > > > > No problem! Thanks for the logs! I think I know what happened: I suppose > > it's "prepared" counter underflow on probe deferral of GPIO driver > > (there are "probe of 44e07000.gpio returned 517" visible). > > Ah. Indeed. The deferral is visible in the logs. > > > > > If you'd still have a chance to test 6.13.0-rc2-next-20241213, > > I believe this was missing in the > > "gpio: omap: save two lines by using devm_clk_get_prepared()": > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 76d5d87e9681..0c30013d2b48 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -1473,8 +1473,6 @@ static int omap_gpio_probe(struct platform_device *pdev) > > if (ret) { > > pm_runtime_put_sync(dev); > > pm_runtime_disable(dev); > > - if (bank->dbck_flag) > > - clk_unprepare(bank->dbck); > > return ret; > > } > > > > @@ -1495,8 +1493,6 @@ static void omap_gpio_remove(struct platform_device *pdev) > > cpu_pm_unregister_notifier(&bank->nb); > > gpiochip_remove(&bank->chip); > > pm_runtime_disable(&pdev->dev); > > - if (bank->dbck_flag) > > - clk_unprepare(bank->dbck); > > } > > > > static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev) > > > > This fixes the boot as you assumed. > I suppose this should be baked in the Bartosz's original patch assumed > it was dropped from the GPIO tree. Yes, this meant to be a "fixup!"... > Furthermore, this seems to be a fix to a hidden problem on original > code. If the original code failed in the clk_prepare() and then deferred > probe(), this same problem should have appeared, right? > > Maybe consider using Fixes - tag even if this and the original change It could be Fixes: 5d9452e7c52a ("gpio: omap: fix clk_prepare/unprepare usage") let's see what Bartosz says on this... > got squashed. Feel free to add a: > > Tested-By: Matti Vaittinen <mazziesaccount@gmail.com> > > if this fix goes to the tree "as is".
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 54c4bfdccf568..57d299d5d0b16 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev) } if (bank->dbck_flag) { - bank->dbck = devm_clk_get(dev, "dbclk"); + bank->dbck = devm_clk_get_prepared(dev, "dbclk"); if (IS_ERR(bank->dbck)) { dev_err(dev, "Could not get gpio dbck. Disable debounce\n"); bank->dbck_flag = false; - } else { - clk_prepare(bank->dbck); } }