[PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

For better build coverage, allow building the gpio-omap driver with
COMPILE_TEST Kconfig option enabled.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 56fee58e281e7..fb923ccd79028 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -530,7 +530,7 @@ config GPIO_OCTEON
 config GPIO_OMAP
 	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
 	default y if ARCH_OMAP
-	depends on ARM
+	depends on ARM || COMPILE_TEST
 	select GENERIC_IRQ_CHIP
 	select GPIOLIB_IRQCHIP
 	help
-- 
2.45.2

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

[Bartosz Golaszewski]

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>
---
 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);
 		}
 	}
 
-- 
2.45.2

Re: [PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Sverdlin, Alexander]

On Tue, 2024-12-03 at 17:41 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better build coverage, allow building the gpio-omap driver with
> COMPILE_TEST Kconfig option enabled.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7..fb923ccd79028 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>  config GPIO_OMAP
>  	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>  	default y if ARCH_OMAP
> -	depends on ARM
> +	depends on ARM || COMPILE_TEST
>  	select GENERIC_IRQ_CHIP
>  	select GPIOLIB_IRQCHIP
>  	help

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

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

[Sverdlin, Alexander]

On Tue, 2024-12-03 at 17:41 +0100, 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>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  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);
>  		}
>  	}

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Andrew Davis]

On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better build coverage, allow building the gpio-omap driver with
> COMPILE_TEST Kconfig option enabled.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/gpio/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7..fb923ccd79028 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>   config GPIO_OMAP
>   	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>   	default y if ARCH_OMAP
> -	depends on ARM
> +	depends on ARM || COMPILE_TEST

Why do we have this depends on ARM at all? It already has that condition
above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.

And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
could just do this:

--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -528,9 +528,9 @@ config GPIO_OCTEON
           family of SOCs.
  
  config GPIO_OMAP
-       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
-       default y if ARCH_OMAP
-       depends on ARM
+       tristate "TI OMAP GPIO support"
+       default y
+       depends on ARCH_OMAP2PLUS || COMPILE_TEST
         select GENERIC_IRQ_CHIP
         select GPIOLIB_IRQCHIP
         help

Andrew

>   	select GENERIC_IRQ_CHIP
>   	select GPIOLIB_IRQCHIP
>   	help

Re: [PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Bartosz Golaszewski]

On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
>
> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > For better build coverage, allow building the gpio-omap driver with
> > COMPILE_TEST Kconfig option enabled.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/gpio/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56fee58e281e7..fb923ccd79028 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -530,7 +530,7 @@ config GPIO_OCTEON
> >   config GPIO_OMAP
> >       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >       default y if ARCH_OMAP
> > -     depends on ARM
> > +     depends on ARM || COMPILE_TEST
>
> Why do we have this depends on ARM at all? It already has that condition
> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
>
> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
> could just do this:
>

I agree we can drop that bit.

> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -528,9 +528,9 @@ config GPIO_OCTEON
>            family of SOCs.
>
>   config GPIO_OMAP
> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> -       default y if ARCH_OMAP
> -       depends on ARM
> +       tristate "TI OMAP GPIO support"
> +       default y
> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST

This would default to y with COMPILE_TEST. We definitely don't want
that. IMO it should be:

tristate "TI OMAP GPIO support"
depends on ARCH_OMAP2PLUS || COMPILE_TEST
default y if ARCH_OMAP2PLUS

Bartosz

Re: [PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Andrew Davis]

On 12/3/24 2:36 PM, Bartosz Golaszewski wrote:
> On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
>>
>> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> For better build coverage, allow building the gpio-omap driver with
>>> COMPILE_TEST Kconfig option enabled.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>>    drivers/gpio/Kconfig | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index 56fee58e281e7..fb923ccd79028 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>>>    config GPIO_OMAP
>>>        tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>>>        default y if ARCH_OMAP
>>> -     depends on ARM
>>> +     depends on ARM || COMPILE_TEST
>>
>> Why do we have this depends on ARM at all? It already has that condition
>> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
>>
>> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
>> could just do this:
>>
> 
> I agree we can drop that bit.
> 
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -528,9 +528,9 @@ config GPIO_OCTEON
>>             family of SOCs.
>>
>>    config GPIO_OMAP
>> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>> -       default y if ARCH_OMAP
>> -       depends on ARM
>> +       tristate "TI OMAP GPIO support"
>> +       default y
>> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST
> 
> This would default to y with COMPILE_TEST. We definitely don't want
> that. IMO it should be:
> 
> tristate "TI OMAP GPIO support"
> depends on ARCH_OMAP2PLUS || COMPILE_TEST
> default y if ARCH_OMAP2PLUS
> 

Looks good to me

Andrew

Re: [PATCH 1/2] gpio: omap: allow building the module with COMPILE_TEST=y

[Bartosz Golaszewski]

On Tue, Dec 3, 2024 at 10:54 PM Andrew Davis <afd@ti.com> wrote:
>
> On 12/3/24 2:36 PM, Bartosz Golaszewski wrote:
> > On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
> >>
> >> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>> For better build coverage, allow building the gpio-omap driver with
> >>> COMPILE_TEST Kconfig option enabled.
> >>>
> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>> ---
> >>>    drivers/gpio/Kconfig | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> >>> index 56fee58e281e7..fb923ccd79028 100644
> >>> --- a/drivers/gpio/Kconfig
> >>> +++ b/drivers/gpio/Kconfig
> >>> @@ -530,7 +530,7 @@ config GPIO_OCTEON
> >>>    config GPIO_OMAP
> >>>        tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >>>        default y if ARCH_OMAP
> >>> -     depends on ARM
> >>> +     depends on ARM || COMPILE_TEST
> >>
> >> Why do we have this depends on ARM at all? It already has that condition
> >> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
> >>
> >> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
> >> could just do this:
> >>
> >
> > I agree we can drop that bit.
> >
> >> --- a/drivers/gpio/Kconfig
> >> +++ b/drivers/gpio/Kconfig
> >> @@ -528,9 +528,9 @@ config GPIO_OCTEON
> >>             family of SOCs.
> >>
> >>    config GPIO_OMAP
> >> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >> -       default y if ARCH_OMAP
> >> -       depends on ARM
> >> +       tristate "TI OMAP GPIO support"
> >> +       default y
> >> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST
> >
> > This would default to y with COMPILE_TEST. We definitely don't want
> > that. IMO it should be:
> >
> > tristate "TI OMAP GPIO support"
> > depends on ARCH_OMAP2PLUS || COMPILE_TEST
> > default y if ARCH_OMAP2PLUS
> >
>
> Looks good to me
>
> Andrew

Nah, this is incorrect, it doesn't build gpio-omap for
omap1_defconfig. I has to depend on ARCH_OMAP under which all omap
platforms fall and they all use this driver.

Bart

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

[Matti Vaittinen]

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

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

[Sverdlin, Alexander]

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

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

[Bartosz Golaszewski]

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

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

[Matti Vaittinen]

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

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

[Sverdlin, Alexander]

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)


-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

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

[Matti Vaittinen]

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.

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

[Matti Vaittinen]

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

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

[Sverdlin, Alexander]

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

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com