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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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?

Other than that:
Acked-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal