Re: AM5749: tty serial 8250 omap driver crash

[Tony Lindgren <tony@atomide.com>]

* 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
>