Re: [PATCH 2/2] gpio: omap: save two lines by using devm_clk_get_prepared()

[Matti Vaittinen <mazziesaccount@gmail.com>]

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

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

[-- Attachment #2: minicom_not_reverted.cap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 112394 bytes --]

[-- Attachment #3: minicom_reverted.cap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 212655 bytes --]