Hi Michal, Thank you for the fast response and the review.
On Thu, Oct 30, 2025 at 10:41 AM Orzel, Michal <[email protected]> wrote: > > > > On 30/10/2025 08:59, Mykola Kvach wrote: > > @Stefano Stabellini @Michal Orzel @Julien Grall @Bertrand Marquis ping > > > > On Thu, Aug 7, 2025 at 8:16 AM Mykola Kvach <[email protected]> wrote: > >> > >> From: Volodymyr Babchuk <[email protected]> > >> > >> 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 <[email protected]> > >> Signed-off-by: Oleksandr Andrushchenko <[email protected]> > >> Signed-off-by: Mykola Kvach <[email protected]> > >> --- > >> In patch v5, there are no changes at all; > >> it was done just to trigger a review. > >> > >> 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); > >> + scif_uart_disable(uart); > >> +} > >> + > >> +static void scif_uart_resume(struct serial_port *port) > >> +{ > >> + struct scif_uart *uart = port->uart; > >> + const struct port_params *params = uart->params; > >> + uint16_t ctrl; > >> + > >> + scif_uart_init_preirq(port); > This will also call scif_uart_disable() that was already invoked during > suspend. > Why do we need to re-disable it when resuming? Thanks for the question. Yes, resume calls scif_uart_init_preirq(), which in turn calls scif_uart_disable(). This is intentional. - While Xen is suspended, EL3 firmware (e.g. TF-A) may use an early or runtime console and reconfigure the SCIF, including enabling TX. Re-disabling gives a quiescent baseline. - PSCI does not guarantee device state across SYSTEM_SUSPEND; device preconditions are outside PSCI's scope. We cannot rely on the state Xen left before suspend. - We reuse scif_uart_init_preirq() on resume to keep a single, well-tested initialization path identical to cold boot. This avoids split logic and keeps behavior consistent. - The extra disable is idempotent and cheap (FIFO/status clear), while preventing spurious data on bring-up. I can add an inline comment to make this explicit: /* On resume, EL3/firmware may have touched SCIF (early/runtime * console). Disable again to start from a clean, quiescent state * before reinit; reuse scif_uart_init_preirq() to keep the cold-boot * sequence. */ > > Other than that: > Acked-by: Michal Orzel <[email protected]> > > ~Michal > Best regards, Mykola
