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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Oleksandr,

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.

>
>
> > +    scif_uart_disable(uart);
> > +}
>
> [snip]

Best regards,
Mykola