[PATCH] bus: ti-sysc: Fix error handling for sysc_check_active_timer() again

[PATCH] bus: ti-sysc: Fix error handling for sysc_check_active_timer() again

[Romain Naour]

From: Romain Naour <romain.naour@skf.com>

sysc_check_active_timer() has been introduced by 6cfcd5563b4f
("clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4")
and initially returned -EBUSY to ignore timers tagged with no-reset
and no-idle.

But the return code has been updated from -EBUSY to -ENXIO by
65fb73676112 ("bus: ti-sysc: suppress err msg for timers used as clockevent/source")
and introduced a regression fixed by 06a089ef6449
("bus: ti-sysc: Fix error handling for sysc_check_active_timer()")
since sysc_probe() was still checking for -EBUSY.

Finally the sysc_check_active_timer() return code was reverted
back to -EBUSY by a12315d6d270
("bus: ti-sysc: Make omap3 gpt12 quirk handling SoC specific") except
for SOC_3430.

Now sysc_check_active_timer() may return ENXIO for SOC_3430 and
EBUSY for all other SoC.

But sysc_probe() still check for -ENXIO leading to the following
errors in dmesg on AM57xx:

ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target)
ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target)
ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target)

Fix this by checking for both error code...

Signed-off-by: Romain Naour <romain.naour@skf.com>
---
 drivers/bus/ti-sysc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
index d57bc066dce6..0c6d5e3d5dc7 100644
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -3314,7 +3314,7 @@ static int sysc_probe(struct platform_device *pdev)
 		return error;
 
 	error = sysc_check_active_timer(ddata);
-	if (error == -ENXIO)
+	if ((error == -ENXIO) || (error == -EBUSY))
 		ddata->reserved = true;
 	else if (error)
 		return error;
-- 
2.43.0

Re: [PATCH] bus: ti-sysc: Fix error handling for sysc_check_active_timer() again

[Andreas Kemnade]

Hi,

On Fri, 22 Dec 2023 17:37:10 +0100
Romain Naour <romain.naour@smile.fr> wrote:

> From: Romain Naour <romain.naour@skf.com>
> 
> sysc_check_active_timer() has been introduced by 6cfcd5563b4f
> ("clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4")
> and initially returned -EBUSY to ignore timers tagged with no-reset
> and no-idle.
> 
> But the return code has been updated from -EBUSY to -ENXIO by
> 65fb73676112 ("bus: ti-sysc: suppress err msg for timers used as clockevent/source")
> and introduced a regression fixed by 06a089ef6449
> ("bus: ti-sysc: Fix error handling for sysc_check_active_timer()")
> since sysc_probe() was still checking for -EBUSY.
> 
> Finally the sysc_check_active_timer() return code was reverted
> back to -EBUSY by a12315d6d270
> ("bus: ti-sysc: Make omap3 gpt12 quirk handling SoC specific") except
> for SOC_3430.
> 
> Now sysc_check_active_timer() may return ENXIO for SOC_3430 and
> EBUSY for all other SoC.
> 
> But sysc_probe() still check for -ENXIO leading to the following
> errors in dmesg on AM57xx:
> 
> ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target)
> ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target)
> ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target)
> 
> Fix this by checking for both error code...
> 
Well, fix what? As long as -EBUSY comes form sysc_check_active_timer(), there
is no problem besides the noise. So clearly state what you want to fix,
so is it only the noise.
Of course I would also like the noise to be gone. I also stumbled across this.
Bringing this to discussion is of course good.

Changing it to -ENXIO has side effects as more lines are executed and the
device is touched although it might be already in use by dmtimer_systimer.

As far as I understand things: there are broken timers, timers used by clocksource
and timers generally useable. And we return -ENXIO for the broken ones... The
main issue here is that this needs more documentation/comments.
I might of course be wrong...

Regards,
Andreas

Re: [PATCH] bus: ti-sysc: Fix error handling for sysc_check_active_timer() again

[Romain Naour]

Hello Andreas,

Le 23/12/2023 à 23:41, Andreas Kemnade a écrit :
> Hi,
> 
> On Fri, 22 Dec 2023 17:37:10 +0100
> Romain Naour <romain.naour@smile.fr> wrote:
> 
>> From: Romain Naour <romain.naour@skf.com>
>>
>> sysc_check_active_timer() has been introduced by 6cfcd5563b4f
>> ("clocksource/drivers/timer-ti-dm: Fix suspend and resume for am3 and am4")
>> and initially returned -EBUSY to ignore timers tagged with no-reset
>> and no-idle.
>>
>> But the return code has been updated from -EBUSY to -ENXIO by
>> 65fb73676112 ("bus: ti-sysc: suppress err msg for timers used as clockevent/source")
>> and introduced a regression fixed by 06a089ef6449
>> ("bus: ti-sysc: Fix error handling for sysc_check_active_timer()")
>> since sysc_probe() was still checking for -EBUSY.
>>
>> Finally the sysc_check_active_timer() return code was reverted
>> back to -EBUSY by a12315d6d270
>> ("bus: ti-sysc: Make omap3 gpt12 quirk handling SoC specific") except
>> for SOC_3430.
>>
>> Now sysc_check_active_timer() may return ENXIO for SOC_3430 and
>> EBUSY for all other SoC.
>>
>> But sysc_probe() still check for -ENXIO leading to the following
>> errors in dmesg on AM57xx:
>>
>> ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target)
>> ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target)
>> ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target)
>>
>> Fix this by checking for both error code...
>>
> Well, fix what? As long as -EBUSY comes form sysc_check_active_timer(), there
> is no problem besides the noise. So clearly state what you want to fix,
> so is it only the noise.
> Of course I would also like the noise to be gone. I also stumbled across this.
> Bringing this to discussion is of course good.

I'm working on a custom board and such "noise" makes difficult to know if the
issue come from the board hardware, the SoC, the devicetree, the TI driver or
the kernel itself.

> 
> Changing it to -ENXIO has side effects as more lines are executed and the
> device is touched although it might be already in use by dmtimer_systimer.

The previous fix [1] doesn't say anything about the -EBUSY case and seems to
forgot updating the error handling for sysc_check_active_timer() (similar to [2])

> 
> As far as I understand things: there are broken timers, timers used by clocksource
> and timers generally useable. And we return -ENXIO for the broken ones... The
> main issue here is that this needs more documentation/comments.
> I might of course be wrong...

I get it now, the last commit [1] actually revert the commit [3] (removing the
error message) by adding back -EBUSY as intended initially [4] for all TI SoC
except for SOC_3430.

sysc_check_active_timer() returning -EBUSY affect 3 timers (timer1 (clockevent),
timer15, timer16) used on AM57xx [5].

ti-sysc: probe of 4ae18000.target-module failed with error -16 (timer1_target)
ti-sysc: probe of 4882c000.target-module failed with error -16 (timer15_target)
ti-sysc: probe of 4882e000.target-module failed with error -16 (timer6_target)

I'm agree, this issue deserve more documentation/comments and it would be better
to avoid such noise.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=a12315d6d27093392b6c634e1d35a59f1d1f7a59

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=e0b603c89a931790dc54b9d6b14b3ec45a82a888

[3]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=65fb73676112f6fd107c5e542b2cbcfb206fe881

[4]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=6cfcd5563b4fadbf49ba8fa481978e5e86d30322

[5]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/dra7.dtsi?h=v6.1.73#n1331

Best regards,
Romain

> 
> Regards,
> Andreas