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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Ayan

On Fri, Jun 13, 2025 at 5:36 PM Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
>
> Hi Mykola,
>
> On 06/06/2025 11:11, Mykola Kvach wrote:
> > CAUTION: This message has originated from an External Source. Please use 
> > proper judgment and caution when opening attachments, clicking links, or 
> > responding to this email.
> >
> >
> > From: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> >
> > The changes have been tested only on the Renesas R-Car H3 Starter Kit board.
>
> The commit message need to explain what the change is and why it is needed.

Thanks for the feedback. I thought the information from the commit
message title would be enough.

I will add a few more words to clarify what the change is and why it's
needed to the commit message body.

>
> Also ...
>
> >
> > 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 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);
> > +
> > +    /* Enable TX/RX and Error Interrupts  */
> > +    ctrl = scif_readw(uart, SCIF_SCSCR);
> > +    scif_writew(uart, SCIF_SCSCR, ctrl | params->irq_flags);
>
> If you can give reference to a public doc which describe these
> registers, it will be great.

I don't think I can share documentation for the board I used for testing
this commit.

However, I searched for Linux boards that use the same driver and SCIF
register set with available documentation, and I found a few boards with
open documentation, for example, RZ/G1H or RZ/G1M:

https://www.renesas.com/en/products/microcontrollers-microprocessors/rz-mpus/rzg1m-ultra-high-performance-microprocessors-15ghz-dual-core-arm-cortex-a15-cpus-3d-graphics-and-video

>
> Otherwise, the changes look ok to me.
>
> > +}
> > +
> > +#endif /* CONFIG_SYSTEM_SUSPEND */
> > +
> >   static struct uart_driver __read_mostly scif_uart_driver = {
> >       .init_preirq  = scif_uart_init_preirq,
> >       .init_postirq = scif_uart_init_postirq,
> > @@ -281,6 +313,10 @@ static struct uart_driver __read_mostly 
> > scif_uart_driver = {
> >       .start_tx     = scif_uart_start_tx,
> >       .stop_tx      = scif_uart_stop_tx,
> >       .vuart_info   = scif_vuart_info,
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    .suspend      = scif_uart_suspend,
> > +    .resume       = scif_uart_resume,
> > +#endif
> >   };
> >
> >   static const struct dt_device_match scif_uart_dt_match[] __initconst =
> - Ayan
> > --
> > 2.48.1
> >
> >

Best regards,
Mykola