Re: [PATCH v5] xen/char: implement suspend/resume calls for SCIF driver

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 29.08.25 00:36, Mykola Kvach wrote:
> Hi Oleksandr,

Hello Mykola

> Thank you for your review.
>
> On Tue, Aug 26, 2025 at 6:51 PM Oleksandr Tyshchenko
> <olekstysh@xxxxxxxxx> wrote:
> > On 07.08.25 08:16, Mykola Kvach wrote:
> >
> >
> > Hello Mykola,
> >
> > In general patch looks good to me, just one question below ...
> >
> > > From: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > >
> > > Implement suspend and resume callbacks for the SCIF UART driver,
> > > enabled when CONFIG_SYSTEM_SUSPEND is set. This allows proper
> > > handling of UART state across system suspend/resume cycles.
> > >
> > > Tested on Renesas R-Car H3 Starter Kit.
> > >
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > > ---
> > > In patch v5, there are no changes at all;
> > > it was done just to trigger a review.
> > I think, you could ping on V4.
> >
> >
> > > In patch v4, enhance commit message, no functional changes
> > >
> > > In patch v2, I just added a CONFIG_SYSTEM_SUSPEND check around
> > > the suspend/resume functions in the SCIF driver.
> > > ---
> > >    xen/drivers/char/scif-uart.c | 40 ++++++++++++++++++++++++++++++++++--
> > >    1 file changed, 38 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
> > > index 757793ca45..888821a3b8 100644
> > > --- a/xen/drivers/char/scif-uart.c
> > > +++ b/xen/drivers/char/scif-uart.c
> > > @@ -139,9 +139,8 @@ static void scif_uart_interrupt(int irq, void *data)
> > >        }
> > >    }
> > >
> > > -static void __init scif_uart_init_preirq(struct serial_port *port)
> > > +static void scif_uart_disable(struct scif_uart *uart)
> > >    {
> > > -    struct scif_uart *uart = port->uart;
> > >        const struct port_params *params = uart->params;
> > >
> > >        /*
> > > @@ -155,6 +154,14 @@ static void __init scif_uart_init_preirq(struct 
> > > serial_port *port)
> > >
> > >        /* Reset TX/RX FIFOs */
> > >        scif_writew(uart, SCIF_SCFCR, SCFCR_RFRST | SCFCR_TFRST);
> > > +}
> > > +
> > > +static void scif_uart_init_preirq(struct serial_port *port)
> > > +{
> > > +    struct scif_uart *uart = port->uart;
> > > +    const struct port_params *params = uart->params;
> > > +
> > > +    scif_uart_disable(uart);
> > >
> > >        /* Clear all errors and flags */
> > >        scif_readw(uart, params->status_reg);
> > > @@ -271,6 +278,31 @@ static void scif_uart_stop_tx(struct serial_port *port)
> > >        scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) & ~SCSCR_TIE);
> > >    }
> > >
> > > +#ifdef CONFIG_SYSTEM_SUSPEND
> > > +
> > > +static void scif_uart_suspend(struct serial_port *port)
> > > +{
> > > +    struct scif_uart *uart = port->uart;
> > > +
> > > +    scif_uart_stop_tx(port);
> >    ... I wonder, whether the call above (that disables Transmit
> > interrupt) is really needed as the call below disables all interrupts
> > anyway?
> I have checked the relevant documentation and did not find any requirement
> to clear TIE before disabling the rest of the SCSCR bits, according to the
> R-Car Series, 3rd Generation User’s Manual: Hardware, Rev. 0.52.
>
> However, based on how the serial subsystem works, I believe this call is
> justified.
>
> Disabling TX IRQs before fully stopping the serial prevents new data
> from being added to the FIFO by the IRQ handler during suspend (see
> serial_tx_interrupt). This ensures the FIFO is flushed (loop inside
> scif_uart_disable) and no further transmissions occur.
>
> If a flush handler were implemented for this driver, we could avoid
> calling scif_uart_stop_tx and remove the loop inside scif_uart_disable,
> as the flush handler would ensure the FIFO is empty after any invocation
> of serial_tx_interrupt.

thanks for the investigation, the explanation is ok to me.

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

> > > +    scif_uart_disable(uart);
> > > +}
> > [snip]
> Best regards,
> Mykola