AM5749: tty serial 8250 omap driver crash

AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hi,

I noticed a kernel crash while opening a serial port connected to a GPS device.
I initially reported the issue to the TI forum [1] but the issue seems related
to the 8250_omap driver.

Using the Linux kernel 4.19-rt provided by the TISDK 6.03, I was able to use a
GPS device connected to the uart4 of the AM5749 device (custom board).
As a basic test I'm using gpscat, gpsmon or gpsctl to open the /dev/gnss0 interface.

Note: The UART4 doesn't use RTS/CTS signals, Only Tx and Rx are defined in the
pinmux project (SW flow control).

Now I'm using a kernel v5.10.87-rt59 from the ti-linux branch [2] but I noticed
a kernel crash while opening /dev/gnss0:

__irq_svc from mem_serial_in+0x4/0x1c
mem_serial_in from omap8250_set_mctrl+0x38/0xa0
omap8250_set_mctrl from uart_update_mctrl+0x4c/0x58
uart_update_mctrl from uart_dtr_rts+0x60/0xa8
uart_dtr_rts from tty_port_block_til_ready+0xd0/0x2a8
tty_port_block_til_ready from uart_open+0x14/0x1c
uart_open from ttyport_open+0x64/0x148
ttyport_open from serdev_device_open+0x28/0xb0
serdev_device_open from gnss_serial_open+0x14/0x98
gnss_serial_open from gnss_open+0x68/0xb4
gnss_open from chrdev_open+0xa8/0x1a0
chrdev_open from do_dentry_open+0x110/0x3ec
do_dentry_open from path_openat+0xb64/0xd6c
path_openat from do_filp_open+0x80/0xf4
do_filp_open from do_sys_openat2+0x304/0x3d8
do_sys_openat2 from do_sys_open+0x7c/0xcc
do_sys_open from ret_fast_syscall+0x0/0x4c

It's not 100% reproducible but if you try 10 times the same command, the kernel
will crash.

It seems that the driver fail to read the UART_LCR register from
omap8250_set_mctrl():

"lcr = serial_in(up, UART_LCR);"

PC is at mem_serial_in+0x2c/0x30
LR is at omap8250_set_mctrl+0x48/0xb0

The problem only occurs with a -rt kernel, I tried with several kernel version:
5.10-rt, 5.15-rt and 5.17-rt.

I'm not able to reproduce the issue with a standard kernel.

While looking at the git history, I noticed this commit [3] about "flakey idling
of uarts and stop using swsup_sidle_act".

So I removed the SYSC_QUIRK for uart IP revision 0x50411e03 and it fixed my issue.

Before that, I tried to revert to SYSC_QUIRK_SWSUP_SIDLE_ACT but the kernel
crash again. Adding "ti,no-idle" to the ti,sysc node parent of uart4 doesn't
solve the issue either.

AFIU, the issue is related to auto suspend (smart-idle) present in uart IPs used
by TI in Sitara AM57xx cpu devices.

The problem is that the uart4 is in idle mode by default since it's not a serial
console.
 As soon as we use gpsmon on /dev/gnss0, the uart IP is switched to operational mode
.
The SYSC_QUIRK is reading the IP revision of the uart4 at address 0x4806E050.

Reading at this address with devmem2 while the gpsmon is not started trigger a
kernel oops:

devmem2 0x4806E050 w



To read this register correctly we have to start gpsmon first (opening /dev/gnss0).

Is the SYSC_QUIRK for omap4 still needed ? Is it safe to remove it ?
It seems this issue was introduced while dropping the legacy platform data
(between 4.19 and 5.4 kernels).

[1]
https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1074192/am5749-tty-serial-8250-omap-driver-crash
[2]
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-5.10.y&id=892cf512e34e60123e043f88bbb80
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=0d83aec6e0102e014eafdd453bdbc61b4d193029

Best regards,
Romain

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

Hi,

* Romain Naour <romain.naour@smile.fr> [220204 13:39]:
> It seems that the driver fail to read the UART_LCR register from
> omap8250_set_mctrl():
> 
> "lcr = serial_in(up, UART_LCR);"
> 
> PC is at mem_serial_in+0x2c/0x30
> LR is at omap8250_set_mctrl+0x48/0xb0
> 
> The problem only occurs with a -rt kernel, I tried with several kernel version:
> 5.10-rt, 5.15-rt and 5.17-rt.
> 
> I'm not able to reproduce the issue with a standard kernel.

Interesting, what's the exception you get with the -rt kernel? Is it an
unhandled external abort or something else?

> While looking at the git history, I noticed this commit [3] about "flakey idling
> of uarts and stop using swsup_sidle_act".
> 
> So I removed the SYSC_QUIRK for uart IP revision 0x50411e03 and it fixed my issue.

Hmm.

> Is the SYSC_QUIRK for omap4 still needed ? Is it safe to remove it ?
> It seems this issue was introduced while dropping the legacy platform data
> (between 4.19 and 5.4 kernels).

AFAIK it's still needed, but maybe we can disable it for am57xx though.

Regards,

Tony

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 07/02/2022 à 09:04, Tony Lindgren a écrit :
> Hi,
> 
> * Romain Naour <romain.naour@smile.fr> [220204 13:39]:
>> It seems that the driver fail to read the UART_LCR register from
>> omap8250_set_mctrl():
>>
>> "lcr = serial_in(up, UART_LCR);"
>>
>> PC is at mem_serial_in+0x2c/0x30
>> LR is at omap8250_set_mctrl+0x48/0xb0
>>
>> The problem only occurs with a -rt kernel, I tried with several kernel version:
>> 5.10-rt, 5.15-rt and 5.17-rt.
>>
>> I'm not able to reproduce the issue with a standard kernel.
> 
> Interesting, what's the exception you get with the -rt kernel? Is it an
> unhandled external abort or something else?

"asynchronous external abort"

Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
pgd = bfdf645d
[00000000] *pgd=862cf003, *pmd=27df1b003
Internal error: : 1211 [#1] PREEMPT_RT SMP ARM
Modules linked in: cmac algif_hash cbc aes_arm_bs crypto_simd cryptd
algif_skcipher af_alg usbhid prueth xhci_plat_hcd irq_pruss_intc xhci_hcd
usbcore pru_rproc icss_iep pvrsr
vkm(O) omap_wdt phy_omap_usb2 ahci_platform libahci_platform omap_aes_driver
pruss libahci libata dwc3 roles udc_core usb_common wl18xx wlcore mac80211
ti_vpe ti_sc ti_csc ti_vpdma dwc3_omap
 wlcore_sdio hci_uart btbcm bluetooth omap_des ecdh_generic libdes ecc
omap_crypto omap_sham crypto_engine omap_remoteproc sch_fq_codel
CPU: 0 PID: 377 Comm: gpsmon Tainted: G        W  O      5.10.87-rt59+ #97
Hardware name: Generic DRA74X (Flattened Device Tree)
PC is at omap8250_set_mctrl+0x38/0xa0
LR is at omap8250_set_mctrl+0x38/0xa0
pc : [<c065f388>]    lr : [<c065f388>]    psr: 60000013
sp : c6327ca0  ip : c6327c74  fp : c4754500
r10: c6327f10  r9 : 00000000  r8 : c22698c8
r7 : ffffe000  r6 : c205ac40  r5 : 00000006  r4 : c12eccd8
r3 : fa06e00c  r2 : 00000002  r1 : 00000003  r0 : 00000000
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 30c5387d  Table: 859817c0  DAC: fffffffd
Process gpsmon (pid: 377, stack limit = 0xaa83ac51)
Stack: (0xc6327ca0 to 0xc6328000)

Sometime after the trace, the kernel panic due to an "exception in interrupt"

[<c024d894>] (__task_rq_lock) from [<c0253148>] (rt_mutex_setprio+0x54/0x4b8)
[<c0253148>] (rt_mutex_setprio) from [<c0276374>]
(task_blocks_on_rt_mutex+0x2a4/0x374)
[<c0276374>] (task_blocks_on_rt_mutex) from [<c0ac7888>]
(rt_spin_lock_slowlock_locked+0xb8/0x2c4)
[<c0ac7888>] (rt_spin_lock_slowlock_locked) from [<c0ac7ae8>]
(rt_spin_lock_slowlock+0x54/0x84)
[<c0ac7ae8>] (rt_spin_lock_slowlock) from [<c0ac9524>] (rt_spin_lock+0x50/0x5c)
[<c0ac9524>] (rt_spin_lock) from [<c0661034>] (omap8250_irq+0x48/0x350)
[<c0661034>] (omap8250_irq) from [<c027e490>] (irq_forced_thread_fn+0x28/0x98)
[<c027e490>] (irq_forced_thread_fn) from [<c027e830>] (irq_thread+0x12c/0x214)
[<c027e830>] (irq_thread) from [<c024704c>] (kthread+0x18c/0x1dc)
[<c024704c>] (kthread) from [<c0200140>] (ret_from_fork+0x14/0x34)

I guess it's due to the previous issue in omap8250_set_mctrl().

> 
>> While looking at the git history, I noticed this commit [3] about "flakey idling
>> of uarts and stop using swsup_sidle_act".
>>
>> So I removed the SYSC_QUIRK for uart IP revision 0x50411e03 and it fixed my issue.
> 
> Hmm.
> 
>> Is the SYSC_QUIRK for omap4 still needed ? Is it safe to remove it ?
>> It seems this issue was introduced while dropping the legacy platform data
>> (between 4.19 and 5.4 kernels).
> 
> AFAIK it's still needed, but maybe we can disable it for am57xx though.

Since I removed the quirk I have other issues while using the serial interface.

I had once a backtrace related to omap_8250_rx_dma_flush with
CONFIG_SERIAL_8250_DMA enabled.

WARNING: CPU: 0 PID: 449 at drivers/tty/serial/8250/8250_omap.c:916
omap_8250_rx_dma_flush+0xec/0xf4
Modules linked in: cmac algif_hash aes_arm aes_generic algif_skcipher af_alg
usbhid xhci_plat_hcd xhci_hcd usbcore irq_pruss_intc prueth pru_rproc icss_iep
omap_wdt pvrsrvkm(
O) phy_omap_usb2 ahci_platform libahci_platform omap_aes_driver pruss libahci
libata dwc3 roles udc_core usb_common wl18xx wlcore mac80211 sha256_generic
libsha256 sha256_arm cfg80211 ti_vp
e ti_sc ti_csc ti_vpdma dwc3_omap wlcore_sdio hci_uart btbcm bluetooth omap_hdq
omap_des ecdh_generic omap_crypto ecc wire libdes libaes omap_sham crypto_engine
sch_fq_codel
CPU: 0 PID: 449 Comm: irq/122-4806e00 Tainted: G           O      5.10.87-rt59+ #91
Hardware name: Generic DRA74X (Flattened Device Tree)
[<c020e19c>] (unwind_backtrace) from [<c0209ef0>] (show_stack+0x10/0x14)
[<c0209ef0>] (show_stack) from [<c0b064b8>] (dump_stack+0x98/0xac)
[<c0b064b8>] (dump_stack) from [<c0b02410>] (__warn+0xcc/0xe4)
[<c0b02410>] (__warn) from [<c0b0248c>] (warn_slowpath_fmt+0x64/0xc8)
[<c0b0248c>] (warn_slowpath_fmt) from [<c06ae5c4>]
(omap_8250_rx_dma_flush+0xec/0xf4)
[<c06ae5c4>] (omap_8250_rx_dma_flush) from [<c06b0610>] (omap8250_irq+0x34c/0x350)
[<c06b0610>] (omap8250_irq) from [<c02836a0>] (irq_forced_thread_fn+0x28/0x98)
[<c02836a0>] (irq_forced_thread_fn) from [<c0283a40>] (irq_thread+0x12c/0x214)
[<c0283a40>] (irq_thread) from [<c0248d94>] (kthread+0x18c/0x1dc)
[<c0248d94>] (kthread) from [<c0200140>] (ret_from_fork+0x14/0x34)
Exception stack(0xc38b1fb0 to 0xc38b1ff8)
1fa0:                                     00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000

To ease investigation, I disabled CONFIG_SERIAL_8250_DMA for now.


I noticed other side effect when opening the serial interface:

omap8250 4806e000.serial: Errata i202: timedout 0

cpsw-switch 48484000.switch: cpts: obtain a time stamp timeout

sched: RT throttling activated

thermal thermal_zone5: failed to read out thermal zone (-121)

It takes several seconds to open the serial interface, something hang somewhere
in the kernel.

Maybe there is something wrong with the smart-standby or smart-idle feature in
the UART IP ? I'm not sure.

Are you able to reproduce it ?
Maybe on a IDK574 or a Beaglebone-AI board ?

Best regards,
Romain


> 
> Regards,
> 
> Tony

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

Hi,

* Romain Naour <romain.naour@smile.fr> [220209 09:13]:
> Le 07/02/2022 à 09:04, Tony Lindgren a écrit :
> > Interesting, what's the exception you get with the -rt kernel? Is it an
> > unhandled external abort or something else?
> 
> "asynchronous external abort"

OK

> Maybe there is something wrong with the smart-standby or smart-idle feature in
> the UART IP ? I'm not sure.

Could be that too, but maybe it's as simple as the patch below. Care to
give it a try?

> Are you able to reproduce it ?
> Maybe on a IDK574 or a Beaglebone-AI board ?

Not sure why I'm not seeing this one with my test systems.. Have not tried
with the RT patches for a while though.

Regards,

Tony

8< -------
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -163,6 +163,8 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	struct omap8250_priv *priv = up->port.private_data;
 	u8 lcr;
 
+	pm_runtime_get_sync(port->dev);
+
 	serial8250_do_set_mctrl(port, mctrl);
 
 	if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) {
@@ -179,6 +181,9 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		serial_out(up, UART_EFR, priv->efr);
 		serial_out(up, UART_LCR, lcr);
 	}
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
 }
 
 /*

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 10/02/2022 à 13:05, Tony Lindgren a écrit :
> Hi,
> 
> * Romain Naour <romain.naour@smile.fr> [220209 09:13]:
>> Le 07/02/2022 à 09:04, Tony Lindgren a écrit :
>>> Interesting, what's the exception you get with the -rt kernel? Is it an
>>> unhandled external abort or something else?
>>
>> "asynchronous external abort"
> 
> OK
> 
>> Maybe there is something wrong with the smart-standby or smart-idle feature in
>> the UART IP ? I'm not sure.
> 
> Could be that too, but maybe it's as simple as the patch below. Care to
> give it a try?

The serial console stop working during the kernel boot with this patch applied.

> 
>> Are you able to reproduce it ?
>> Maybe on a IDK574 or a Beaglebone-AI board ?
> 
> Not sure why I'm not seeing this one with my test systems.. Have not tried
> with the RT patches for a while though.

The RT patches really makes a difference here.

> 
> Regards,
> 
> Tony
> 
> 8< -------
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -163,6 +163,8 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	struct omap8250_priv *priv = up->port.private_data;
>  	u8 lcr;
>  
> +	pm_runtime_get_sync(port->dev);
> +
>  	serial8250_do_set_mctrl(port, mctrl);
>  
>  	if (!mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS)) {
> @@ -179,6 +181,9 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  		serial_out(up, UART_EFR, priv->efr);
>  		serial_out(up, UART_LCR, lcr);
>  	}
> +
> +	pm_runtime_mark_last_busy(port->dev);
> +	pm_runtime_put_autosuspend(port->dev);

This suspend the serial debug console on uart3.

Best regards,
Romain


>  }
>  
>  /*

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Romain Naour <romain.naour@smile.fr> [220211 10:10]:
> Le 10/02/2022 à 13:05, Tony Lindgren a écrit :
> > Could be that too, but maybe it's as simple as the patch below. Care to
> > give it a try?
> 
> The serial console stop working during the kernel boot with this patch applied.

Yeah and we already have the serdev taking case of PM runtime here.

> >> Are you able to reproduce it ?
> >> Maybe on a IDK574 or a Beaglebone-AI board ?
> > 
> > Not sure why I'm not seeing this one with my test systems.. Have not tried
> > with the RT patches for a while though.
> 
> The RT patches really makes a difference here.

Looks like the following script to just toggle the module state locks
up things for me on beagle-x15 very fast. So yeah now I'm able to
reproduce the issue. Seems like the module is not ready right after
enabling it live we've seen for dra7 iva for example.

Regards,

Tony

8< ------
#!/bin/sh

# Test rebind for serial console uart3 ttyS2, run from an ssh session
module="48020050.target-module"
driver="/sys/bus/platform/drivers/ti-sysc"

# Detach all kernel serial consoles
consoles=$(find /sys/bus/platform/devices/4*.serial/ -name console)
for console in ${consoles}; do
        echo -n N > ${console}
done

# Configure PM runtime autosuspend for all uarts
uarts=$(find /sys/bus/platform/devices/4*.serial/power/ -type d)
for uart in $uarts; do
        echo -n 3000 > $uart/autosuspend_delay_ms
        echo -n enabled > $uart/wakeup
        echo -n auto > $uart/control
done

# Configure wake-up from suspend for all uarts
uarts=$(find /sys/class/tty/tty[SO]*/power/ -type d 2>/dev/null)
for uart in $uarts; do
        echo -n enabled > $uart/wakeup
done

# Keep rebinding uart3 in a loop
while true; do
        echo ${module} > ${driver}/bind
        echo ${module} > ${driver}/unbind
done

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [220214 07:43]:
> Looks like the following script to just toggle the module state locks
> up things for me on beagle-x15 very fast. So yeah now I'm able to
> reproduce the issue. Seems like the module is not ready right after
> enabling it live we've seen for dra7 iva for example.

Looks like the following patch is also needed for uarts to avoid unbind
clock_unprepare warnings. But even with this patch dra7 uarts won't behave.
On unbind, there will be a clock "l4per-clkctrl:0128:0: failed to disable"
warning. Looks like after that any following clock enable does not seem to
work and that will cause the register access errors.

Looks like this is a dra7 specific issue as a similar test script on omap4
duovero keeps on going instead.

Regards,

Tony

8< --------------
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -3381,7 +3381,9 @@ static int sysc_remove(struct platform_device *pdev)
 	struct sysc *ddata = platform_get_drvdata(pdev);
 	int error;
 
-	cancel_delayed_work_sync(&ddata->idle_work);
+	/* Device can still be enabled, see deferred idle quirk in probe */
+	if (cancel_delayed_work_sync(&ddata->idle_work))
+		ti_sysc_idle(&ddata->idle_work.work);
 
 	error = pm_runtime_resume_and_get(ddata->dev);
 	if (error < 0) {

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 14/02/2022 à 14:08, Tony Lindgren a écrit :
> * Tony Lindgren <tony@atomide.com> [220214 07:43]:
>> Looks like the following script to just toggle the module state locks
>> up things for me on beagle-x15 very fast. So yeah now I'm able to
>> reproduce the issue. Seems like the module is not ready right after
>> enabling it live we've seen for dra7 iva for example.
> 
> Looks like the following patch is also needed for uarts to avoid unbind
> clock_unprepare warnings. But even with this patch dra7 uarts won't behave.
> On unbind, there will be a clock "l4per-clkctrl:0128:0: failed to disable"
> warning. Looks like after that any following clock enable does not seem to
> work and that will cause the register access errors.
> 
> Looks like this is a dra7 specific issue as a similar test script on omap4
> duovero keeps on going instead.

Thanks for the help and the patch!

I removed my patch removing the quirk and applied you patch but I can still
reproduce the issue.

Do you need some info about the kernel configuration?

Best regards,
Romain


> 
> Regards,
> 
> Tony
> 
> 8< --------------
> diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> --- a/drivers/bus/ti-sysc.c
> +++ b/drivers/bus/ti-sysc.c
> @@ -3381,7 +3381,9 @@ static int sysc_remove(struct platform_device *pdev)
>  	struct sysc *ddata = platform_get_drvdata(pdev);
>  	int error;
>  
> -	cancel_delayed_work_sync(&ddata->idle_work);
> +	/* Device can still be enabled, see deferred idle quirk in probe */
> +	if (cancel_delayed_work_sync(&ddata->idle_work))
> +		ti_sysc_idle(&ddata->idle_work.work);
>  
>  	error = pm_runtime_resume_and_get(ddata->dev);
>  	if (error < 0) {

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Romain Naour <romain.naour@smile.fr> [220216 09:04]:
> Hello,
> 
> Le 14/02/2022 à 14:08, Tony Lindgren a écrit :
> > * Tony Lindgren <tony@atomide.com> [220214 07:43]:
> >> Looks like the following script to just toggle the module state locks
> >> up things for me on beagle-x15 very fast. So yeah now I'm able to
> >> reproduce the issue. Seems like the module is not ready right after
> >> enabling it live we've seen for dra7 iva for example.
> > 
> > Looks like the following patch is also needed for uarts to avoid unbind
> > clock_unprepare warnings. But even with this patch dra7 uarts won't behave.
> > On unbind, there will be a clock "l4per-clkctrl:0128:0: failed to disable"
> > warning. Looks like after that any following clock enable does not seem to
> > work and that will cause the register access errors.
> > 
> > Looks like this is a dra7 specific issue as a similar test script on omap4
> > duovero keeps on going instead.
> 
> Thanks for the help and the patch!
> 
> I removed my patch removing the quirk and applied you patch but I can still
> reproduce the issue.

Yeah issues still exists for sure, looks like also omap4 fails but it just
takes a while to produce the clkctrl disable error. And remove for 8250_omap
is incomplete..

Below is a patch that makes the rebind of kernel serial console behave for me
together with the ti-sysc patch.

Additionally I also need to disable dma for now with:

&uart3 {
	/delete-property/ dma-names;
};

> Do you need some info about the kernel configuration?

Not really but if you can please test again with the ti-sysc patch,
8250_omap patch and with your serdev uart dma disabled with
delete-property?

Regards,

Tony

8< -----------------
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1475,10 +1475,15 @@ static int omap8250_probe(struct platform_device *pdev)
 static int omap8250_remove(struct platform_device *pdev)
 {
 	struct omap8250_priv *priv = platform_get_drvdata(pdev);
+	struct uart_8250_port *up = serial8250_get_port(priv->line);
 
+	pm_runtime_resume_and_get(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	dev_pm_clear_wake_irq(&pdev->dev);
+	cancel_work_sync(&priv->qos_work);
+	cancel_delayed_work(&up->overrun_backoff);
 	serial8250_unregister_port(priv->line);
 	cpu_latency_qos_remove_request(&priv->pm_qos_request);
 	device_init_wakeup(&pdev->dev, false);

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 16/02/2022 à 12:46, Tony Lindgren a écrit :
> * Romain Naour <romain.naour@smile.fr> [220216 09:04]:
>> Hello,
>>
>> Le 14/02/2022 à 14:08, Tony Lindgren a écrit :
>>> * Tony Lindgren <tony@atomide.com> [220214 07:43]:
>>>> Looks like the following script to just toggle the module state locks
>>>> up things for me on beagle-x15 very fast. So yeah now I'm able to
>>>> reproduce the issue. Seems like the module is not ready right after
>>>> enabling it live we've seen for dra7 iva for example.
>>>
>>> Looks like the following patch is also needed for uarts to avoid unbind
>>> clock_unprepare warnings. But even with this patch dra7 uarts won't behave.
>>> On unbind, there will be a clock "l4per-clkctrl:0128:0: failed to disable"
>>> warning. Looks like after that any following clock enable does not seem to
>>> work and that will cause the register access errors.
>>>
>>> Looks like this is a dra7 specific issue as a similar test script on omap4
>>> duovero keeps on going instead.
>>
>> Thanks for the help and the patch!
>>
>> I removed my patch removing the quirk and applied you patch but I can still
>> reproduce the issue.
> 
> Yeah issues still exists for sure, looks like also omap4 fails but it just
> takes a while to produce the clkctrl disable error. And remove for 8250_omap
> is incomplete..

Ok.
> 
> Below is a patch that makes the rebind of kernel serial console behave for me
> together with the ti-sysc patch.
> 
> Additionally I also need to disable dma for now with:
> 
> &uart3 {
> 	/delete-property/ dma-names;
> };

On my side I'm using uart4 but I don't think it makes a difference.

> 
>> Do you need some info about the kernel configuration?
> 
> Not really but if you can please test again with the ti-sysc patch,
> 8250_omap patch and with your serdev uart dma disabled with
> delete-property?

I had a crash but on close path:

[<c06af3b0>] (omap8250_set_mctrl) from [<c069fd38>] (uart_update_mctrl+0x3c/0x48)
[<c069fd38>] (uart_update_mctrl) from [<c06a2ac8>] (uart_dtr_rts+0x54/0x9c)
[<c06a2ac8>] (uart_dtr_rts) from [<c068b0d0>] (tty_port_shutdown+0x78/0x9c)
[<c068b0d0>] (tty_port_shutdown) from [<c068b8ec>] (tty_port_close+0x3c/0x74)
[<c068b8ec>] (tty_port_close) from [<c06b3b4c>] (ttyport_close+0x40/0x58)
[<c06b3b4c>] (ttyport_close) from [<c092aca4>] (gnss_serial_close+0x14/0x24)
[<c092aca4>] (gnss_serial_close) from [<c092a4a0>] (gnss_release+0x44/0x64)
[<c092a4a0>] (gnss_release) from [<c036b7f4>] (__fput+0x78/0x23c)
[<c036b7f4>] (__fput) from [<c0246308>] (task_work_run+0x90/0xbc)
[<c0246308>] (task_work_run) from [<c0209c0c>] (do_work_pending+0x558/0x560)
[<c0209c0c>] (do_work_pending) from [<c02000cc>] (slow_work_pending+0xc/0x20)

My test is starting gpsmon /dev/gnss0 several time.

I hope it help.

Best regards,
Romain


> 
> Regards,
> 
> Tony
> 
> 8< -----------------
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1475,10 +1475,15 @@ static int omap8250_probe(struct platform_device *pdev)
>  static int omap8250_remove(struct platform_device *pdev)
>  {
>  	struct omap8250_priv *priv = platform_get_drvdata(pdev);
> +	struct uart_8250_port *up = serial8250_get_port(priv->line);
>  
> +	pm_runtime_resume_and_get(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	dev_pm_clear_wake_irq(&pdev->dev);
> +	cancel_work_sync(&priv->qos_work);
> +	cancel_delayed_work(&up->overrun_backoff);
>  	serial8250_unregister_port(priv->line);
>  	cpu_latency_qos_remove_request(&priv->pm_qos_request);
>  	device_init_wakeup(&pdev->dev, false);

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Romain Naour <romain.naour@smile.fr> [220216 15:51]:
> Le 16/02/2022 à 12:46, Tony Lindgren a écrit :
> > Not really but if you can please test again with the ti-sysc patch,
> > 8250_omap patch and with your serdev uart dma disabled with
> > delete-property?
> 
> I had a crash but on close path:
> 
> [<c06af3b0>] (omap8250_set_mctrl) from [<c069fd38>] (uart_update_mctrl+0x3c/0x48)
> [<c069fd38>] (uart_update_mctrl) from [<c06a2ac8>] (uart_dtr_rts+0x54/0x9c)
> [<c06a2ac8>] (uart_dtr_rts) from [<c068b0d0>] (tty_port_shutdown+0x78/0x9c)
> [<c068b0d0>] (tty_port_shutdown) from [<c068b8ec>] (tty_port_close+0x3c/0x74)
> [<c068b8ec>] (tty_port_close) from [<c06b3b4c>] (ttyport_close+0x40/0x58)
> [<c06b3b4c>] (ttyport_close) from [<c092aca4>] (gnss_serial_close+0x14/0x24)
> [<c092aca4>] (gnss_serial_close) from [<c092a4a0>] (gnss_release+0x44/0x64)
> [<c092a4a0>] (gnss_release) from [<c036b7f4>] (__fput+0x78/0x23c)
> [<c036b7f4>] (__fput) from [<c0246308>] (task_work_run+0x90/0xbc)
> [<c0246308>] (task_work_run) from [<c0209c0c>] (do_work_pending+0x558/0x560)
> [<c0209c0c>] (do_work_pending) from [<c02000cc>] (slow_work_pending+0xc/0x20)
> 
> My test is starting gpsmon /dev/gnss0 several time.

OK sounds like a good test.

We can have two kind of issues, either runtime PM is not enabled, or
runtime PM is enabled but the uart registers have not been yet inialized.

On probe, omap8250_set_mctrl() can get called with runtime PM enabled, but
without omap8250_restore_regs() having been called yet. It seems we rely
on a bootloader initialized uart register state here currently.

We don't have omap8250_restore_regs() called on probe until at
omap_8250_set_termios(). But before we alread have this:

uart_add_one_port()
  uart_configure_port()
    omap8250_set_mctrl()

So that explains at least some issues. I'll take a look at what's the best
way to fix this.

I'm not quite sure why enabling dma on dra7 causes the clk disable errors,
maybe it's related to priv->delayed_restore set in some cases. Or some
different uart register init for dma.

For your issue with close, I wonder if it's related to autoidling the uarts?
The test script I posted does that for all the uarts, probably best not
to do that until the other issues are sorted out :) If so, maybe the issue
on close is that the uart has already autoidled.

Regards,

Tony

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 17/02/2022 à 09:08, Tony Lindgren a écrit :
> * Romain Naour <romain.naour@smile.fr> [220216 15:51]:
>> Le 16/02/2022 à 12:46, Tony Lindgren a écrit :
>>> Not really but if you can please test again with the ti-sysc patch,
>>> 8250_omap patch and with your serdev uart dma disabled with
>>> delete-property?
>>
>> I had a crash but on close path:
>>
>> [<c06af3b0>] (omap8250_set_mctrl) from [<c069fd38>] (uart_update_mctrl+0x3c/0x48)
>> [<c069fd38>] (uart_update_mctrl) from [<c06a2ac8>] (uart_dtr_rts+0x54/0x9c)
>> [<c06a2ac8>] (uart_dtr_rts) from [<c068b0d0>] (tty_port_shutdown+0x78/0x9c)
>> [<c068b0d0>] (tty_port_shutdown) from [<c068b8ec>] (tty_port_close+0x3c/0x74)
>> [<c068b8ec>] (tty_port_close) from [<c06b3b4c>] (ttyport_close+0x40/0x58)
>> [<c06b3b4c>] (ttyport_close) from [<c092aca4>] (gnss_serial_close+0x14/0x24)
>> [<c092aca4>] (gnss_serial_close) from [<c092a4a0>] (gnss_release+0x44/0x64)
>> [<c092a4a0>] (gnss_release) from [<c036b7f4>] (__fput+0x78/0x23c)
>> [<c036b7f4>] (__fput) from [<c0246308>] (task_work_run+0x90/0xbc)
>> [<c0246308>] (task_work_run) from [<c0209c0c>] (do_work_pending+0x558/0x560)
>> [<c0209c0c>] (do_work_pending) from [<c02000cc>] (slow_work_pending+0xc/0x20)
>>
>> My test is starting gpsmon /dev/gnss0 several time.
> 
> OK sounds like a good test.
> 
> We can have two kind of issues, either runtime PM is not enabled, or
> runtime PM is enabled but the uart registers have not been yet inialized.

I'm using ARCH_OMAP2PLUS_TYPICAL, so at least CONFIG_PM is selected.

> 
> On probe, omap8250_set_mctrl() can get called with runtime PM enabled, but
> without omap8250_restore_regs() having been called yet. It seems we rely
> on a bootloader initialized uart register state here currently.

On u-boot devicetree the uart4 node is missing, but we don't plan to use the gps
from uboot :)
Should I add the uart4 node?

> 
> We don't have omap8250_restore_regs() called on probe until at
> omap_8250_set_termios(). But before we alread have this:
> 
> uart_add_one_port()
>   uart_configure_port()
>     omap8250_set_mctrl()
> 
> So that explains at least some issues. I'll take a look at what's the best
> way to fix this.
> 
> I'm not quite sure why enabling dma on dra7 causes the clk disable errors,
> maybe it's related to priv->delayed_restore set in some cases. Or some
> different uart register init for dma.
> 
> For your issue with close, I wonder if it's related to autoidling the uarts?

Since removing the uart quirk had some other side effect, I did a shameless hack
in 8250_omap.c to disable autosuspend.

-	pm_runtime_put_autosuspend(port->dev);
+	pm_runtime_dont_use_autosuspend(port->dev);

With that the uart is always up and running.

> The test script I posted does that for all the uarts, probably best not
> to do that until the other issues are sorted out :) If so, maybe the issue
> on close is that the uart has already autoidled.

Indeed. But I'm not sure why the autosuspend is enabled by default?

Best regards,
Romain

> 
> Regards,
> 
> Tony

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Romain Naour <romain.naour@smile.fr> [220217 09:09]:
> On u-boot devicetree the uart4 node is missing, but we don't plan to use the gps
> from uboot :)
> Should I add the uart4 node?

There should be no need for that, kernel should be able to initialize it
properly no matter what the state is from the bootloader.

> Since removing the uart quirk had some other side effect, I did a shameless hack
> in 8250_omap.c to disable autosuspend.
> 
> -	pm_runtime_put_autosuspend(port->dev);
> +	pm_runtime_dont_use_autosuspend(port->dev);
> 
> With that the uart is always up and running.

Yes but note that 8250_omap autosuspend does not do anything unless the
timeouts are manually enabled by the userspace. They are initialized to -1
during probe.

> > The test script I posted does that for all the uarts, probably best not
> > to do that until the other issues are sorted out :) If so, maybe the issue
> > on close is that the uart has already autoidled.
> 
> Indeed. But I'm not sure why the autosuspend is enabled by default?

See above, it's always been initialized to -1 by default so it won't
do anything. But if you ran the test script I posted, autosuspend timeout
got enabled for all the uarts. But maybe the issue you posted is yet
another issue that I don't quite understand yet.

To me it seems we should always have runtime PM functions enable the
serial port to usable state and get rid of the conditional enable for
probe and dma.

Regards,

Tony

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello Tony,

Sorry for the delay.

Le 17/02/2022 à 13:58, Tony Lindgren a écrit :
> * Romain Naour <romain.naour@smile.fr> [220217 09:09]:
>> On u-boot devicetree the uart4 node is missing, but we don't plan to use the gps
>> from uboot :)
>> Should I add the uart4 node?
> 
> There should be no need for that, kernel should be able to initialize it
> properly no matter what the state is from the bootloader.

ok

> 
>> Since removing the uart quirk had some other side effect, I did a shameless hack
>> in 8250_omap.c to disable autosuspend.
>>
>> -	pm_runtime_put_autosuspend(port->dev);
>> +	pm_runtime_dont_use_autosuspend(port->dev);
>>
>> With that the uart is always up and running.
> 
> Yes but note that 8250_omap autosuspend does not do anything unless the
> timeouts are manually enabled by the userspace. They are initialized to -1
> during probe.

Actually it's not initialized to -1 on my board, it's initialized to 0. See
commit [1].

I'm starting to think that is an issue when the 8250_omap driver is used with
another driver like the gnss serial driver (using a serdev driver).

Since the commit [1] there is no autosuspend delay at all, I would suggest to
add a default autosuspend delay value. I tested with 200ms based on another example.

diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index ec7304d57f5f..8ba830bd493a 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -108,6 +108,9 @@
 /* RX FIFO occupancy indicator */
 #define UART_OMAP_RX_LVL               0x19

+/* Runtime PM autosuspend timeout: 0ms may trigger wakeup issues */
+#define UART_AUTOSUSPEND_TIMEOUT               200
+
 struct omap8250_priv {
        int line;
        u8 habit;
@@ -1409,6 +1412,8 @@ static int omap8250_probe(struct platform_device *pdev)
         */
        if (!of_get_available_child_count(pdev->dev.of_node))
                pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+       else
+               pm_runtime_set_autosuspend_delay(&pdev->dev,
UART_AUTOSUSPEND_TIMEOUT);

        pm_runtime_irq_safe(&pdev->dev);


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

> 
>>> The test script I posted does that for all the uarts, probably best not
>>> to do that until the other issues are sorted out :) If so, maybe the issue
>>> on close is that the uart has already autoidled.
>>
>> Indeed. But I'm not sure why the autosuspend is enabled by default?
> 
> See above, it's always been initialized to -1 by default so it won't
> do anything. But if you ran the test script I posted, autosuspend timeout
> got enabled for all the uarts. But maybe the issue you posted is yet
> another issue that I don't quite understand yet.
> 
> To me it seems we should always have runtime PM functions enable the
> serial port to usable state and get rid of the conditional enable for
> probe and dma.

I'm not able to reproduce the issue by preventing the device from being power
managed. I tried with both method:

echo "-1" > /sys/bus/platform/devices/4806e000.serial/power/autosuspend_delay_ms

or

echo on > /sys/bus/platform/devices/4806e000.serial/power/control

I also played with your script modified to keep autosuspend_delay_ms to 0
(default on my board), but It doesn't trigger the issue.

Note: /sys/bus/platform/devices/4806e000.serial/power/wakeup doesn't exist here.

If setting autosuspend_delay to 200ms by default is ok, I will send a patch.

Best regards,
Romain

> 
> Regards,
> 
> Tony

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

Hi,

* Romain Naour <romain.naour@smile.fr> [220402 10:13]:
> Le 17/02/2022 à 13:58, Tony Lindgren a écrit :
> > Yes but note that 8250_omap autosuspend does not do anything unless the
> > timeouts are manually enabled by the userspace. They are initialized to -1
> > during probe.
> 
> Actually it's not initialized to -1 on my board, it's initialized to 0. See
> commit [1].

Oh you're right. The default should be initialized to 2000ms though, not 0.

> I'm starting to think that is an issue when the 8250_omap driver is used with
> another driver like the gnss serial driver (using a serdev driver).

Oh yes you are right. We do this conditionally now.

> Since the commit [1] there is no autosuspend delay at all, I would suggest to
> add a default autosuspend delay value. I tested with 200ms based on another example.
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c
> b/drivers/tty/serial/8250/8250_omap.c
> index ec7304d57f5f..8ba830bd493a 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -108,6 +108,9 @@
>  /* RX FIFO occupancy indicator */
>  #define UART_OMAP_RX_LVL               0x19
> 
> +/* Runtime PM autosuspend timeout: 0ms may trigger wakeup issues */
> +#define UART_AUTOSUSPEND_TIMEOUT               200
> +
>  struct omap8250_priv {
>         int line;
>         u8 habit;
> @@ -1409,6 +1412,8 @@ static int omap8250_probe(struct platform_device *pdev)
>          */
>         if (!of_get_available_child_count(pdev->dev.of_node))
>                 pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> +       else
> +               pm_runtime_set_autosuspend_delay(&pdev->dev,
> UART_AUTOSUSPEND_TIMEOUT);
> 
>         pm_runtime_irq_safe(&pdev->dev);

Hmm the value should be set to the default 2000ms already. If it's not, then we
need to find out what is setting it to 0.

For adjusting the timeout, you may want to check the child serdev driver
runtime PM autosuspend timeout, adjusting that seems a better place to
prevent the 8250 idle. Not sure how we should handle the 8250 specific
timeout though based on the serdev driver requirements.

Regards,

Tony


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

Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour]

Hello,

Le 03/05/2022 à 12:05, Tony Lindgren a écrit :
> Hi,
> 
> * Romain Naour <romain.naour@smile.fr> [220402 10:13]:
>> Le 17/02/2022 à 13:58, Tony Lindgren a écrit :
>>> Yes but note that 8250_omap autosuspend does not do anything unless the
>>> timeouts are manually enabled by the userspace. They are initialized to -1
>>> during probe.
>>
>> Actually it's not initialized to -1 on my board, it's initialized to 0. See
>> commit [1].
> 
> Oh you're right. The default should be initialized to 2000ms though, not 0.

How do you know it should use 2000ms by default?

> 
>> I'm starting to think that is an issue when the 8250_omap driver is used with
>> another driver like the gnss serial driver (using a serdev driver).
> 
> Oh yes you are right. We do this conditionally now.
> 
>> Since the commit [1] there is no autosuspend delay at all, I would suggest to
>> add a default autosuspend delay value. I tested with 200ms based on another example.
>>
>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>> b/drivers/tty/serial/8250/8250_omap.c
>> index ec7304d57f5f..8ba830bd493a 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -108,6 +108,9 @@
>>  /* RX FIFO occupancy indicator */
>>  #define UART_OMAP_RX_LVL               0x19
>>
>> +/* Runtime PM autosuspend timeout: 0ms may trigger wakeup issues */
>> +#define UART_AUTOSUSPEND_TIMEOUT               200
>> +
>>  struct omap8250_priv {
>>         int line;
>>         u8 habit;
>> @@ -1409,6 +1412,8 @@ static int omap8250_probe(struct platform_device *pdev)
>>          */
>>         if (!of_get_available_child_count(pdev->dev.of_node))
>>                 pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>> +       else
>> +               pm_runtime_set_autosuspend_delay(&pdev->dev,
>> UART_AUTOSUSPEND_TIMEOUT);
>>
>>         pm_runtime_irq_safe(&pdev->dev);
> 
> Hmm the value should be set to the default 2000ms already. If it's not, then we
> need to find out what is setting it to 0.

I don't see where pm_runtime_set_autosuspend_delay() would be called to set the
autosuspend delay to 0.

2000ms seems to be related to USB_AUTOSUSPEND_DELAY and only relevant for the
usb stack.

Here nothing seems calling pm_runtime_set_autosuspend_delay(), so the
autosuspend delay is still using 0 as default value. I'm not sure that the
serdev driver really handle the autosuspend delay.

Other driver like the omap-sham set the autosuspend delay default value just
after pm_runtime_use_autosuspend(&pdev->dev):

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/crypto/omap-sham.c?h=linux-5.10.y#n2126

> 
> For adjusting the timeout, you may want to check the child serdev driver
> runtime PM autosuspend timeout, adjusting that seems a better place to
> prevent the 8250 idle. Not sure how we should handle the 8250 specific
> timeout though based on the serdev driver requirements.

For now, I'm not sure I need to adjust the timeout.

Best regards,
Romain

> 
> Regards,
> 
> Tony
> 
> 
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=627a545c6bb0c7de09208e6546f5111290477261

Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren]

* Romain Naour <romain.naour@smile.fr> [220504 12:38]:
> Hello,
> 
> Le 03/05/2022 à 12:05, Tony Lindgren a écrit :
> > Hi,
> > 
> > * Romain Naour <romain.naour@smile.fr> [220402 10:13]:
> >> Le 17/02/2022 à 13:58, Tony Lindgren a écrit :
> >>> Yes but note that 8250_omap autosuspend does not do anything unless the
> >>> timeouts are manually enabled by the userspace. They are initialized to -1
> >>> during probe.
> >>
> >> Actually it's not initialized to -1 on my board, it's initialized to 0. See
> >> commit [1].
> > 
> > Oh you're right. The default should be initialized to 2000ms though, not 0.
> 
> How do you know it should use 2000ms by default?

Oh I recalled we had such default value.. Seems I was wrong. Sorry for the
wrong information.

> >> I'm starting to think that is an issue when the 8250_omap driver is used with
> >> another driver like the gnss serial driver (using a serdev driver).
> > 
> > Oh yes you are right. We do this conditionally now.
> > 
> >> Since the commit [1] there is no autosuspend delay at all, I would suggest to
> >> add a default autosuspend delay value. I tested with 200ms based on another example.
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_omap.c
> >> b/drivers/tty/serial/8250/8250_omap.c
> >> index ec7304d57f5f..8ba830bd493a 100644
> >> --- a/drivers/tty/serial/8250/8250_omap.c
> >> +++ b/drivers/tty/serial/8250/8250_omap.c
> >> @@ -108,6 +108,9 @@
> >>  /* RX FIFO occupancy indicator */
> >>  #define UART_OMAP_RX_LVL               0x19
> >>
> >> +/* Runtime PM autosuspend timeout: 0ms may trigger wakeup issues */
> >> +#define UART_AUTOSUSPEND_TIMEOUT               200
> >> +
> >>  struct omap8250_priv {
> >>         int line;
> >>         u8 habit;
> >> @@ -1409,6 +1412,8 @@ static int omap8250_probe(struct platform_device *pdev)
> >>          */
> >>         if (!of_get_available_child_count(pdev->dev.of_node))
> >>                 pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> >> +       else
> >> +               pm_runtime_set_autosuspend_delay(&pdev->dev,
> >> UART_AUTOSUSPEND_TIMEOUT);
> >>
> >>         pm_runtime_irq_safe(&pdev->dev);
> > 
> > Hmm the value should be set to the default 2000ms already. If it's not, then we
> > need to find out what is setting it to 0.
> 
> I don't see where pm_runtime_set_autosuspend_delay() would be called to set the
> autosuspend delay to 0.
> 
> 2000ms seems to be related to USB_AUTOSUSPEND_DELAY and only relevant for the
> usb stack.

OK maybe that's where I got the idea.

> Here nothing seems calling pm_runtime_set_autosuspend_delay(), so the
> autosuspend delay is still using 0 as default value. I'm not sure that the
> serdev driver really handle the autosuspend delay.

OK. Is your serdev driver not configuring the autosuspend value either?

> Other driver like the omap-sham set the autosuspend delay default value just
> after pm_runtime_use_autosuspend(&pdev->dev):
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/crypto/omap-sham.c?h=linux-5.10.y#n2126
> 
> > 
> > For adjusting the timeout, you may want to check the child serdev driver
> > runtime PM autosuspend timeout, adjusting that seems a better place to
> > prevent the 8250 idle. Not sure how we should handle the 8250 specific
> > timeout though based on the serdev driver requirements.
> 
> For now, I'm not sure I need to adjust the timeout.

Well what is the child serdev driver setting the autosuspend timeout to?

That should prevent the parent 8250 device from suspending. If the serdev
driver is not using autosuspend, that should prevent the parent 8250 device
from suspending also until the serdev driver decides to runtime suspend.

I guess I'm not following why the 8250 autosuspend triggers with a serdev
unless your serdev driver runtime suspends.. Or is there maybe some issue
where runtime suspending 8250 still causes register access after that
somehow?

Regards,

Tony


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