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

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

On 03/11/2025 08:30, Mykola Kvach wrote:
> 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.
>  */
No need, your explanation makes sense. Thanks.

~Michal