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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Michal,

Thank you for the fast response and the review.

On Thu, Oct 30, 2025 at 10:41 AM Orzel, Michal <michal.orzel@xxxxxxx> 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 <xakep.amatop@xxxxxxxxx> wrote:
> >>
> >> 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.
> >>
> >> 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 <michal.orzel@xxxxxxx>
>
> ~Michal
>

Best regards,
Mykola