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 <[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.
> */
No need, your explanation makes sense. Thanks.
~Michal
