Re: AM5749: tty serial 8250 omap driver crash

[Romain Naour <romain.naour@smile.fr>]

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