xen/drivers/char/scif-uart.c | 40 ++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)
From: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
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@epam.com>
Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
In patch v2, I just added a CONFIG_SYSTEM_SUSPEND check around
the suspend/resume functions in the SCIF driver.
In patch v4, enhance commit message, no functional changes
---
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);
+}
+
+#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 =
--
2.48.1On 24.06.2025 09:18, Mykola Kvach wrote:
> @@ -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
> };
As this being put inside #ifdef was to be expected, imo a prereq change is to
also make the struct fields conditional in xen/console.h. I think I did even
comment to this effect back at the time.
Jan
Hi Jan,
On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.06.2025 09:18, Mykola Kvach wrote:
> > @@ -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
> > };
>
> As this being put inside #ifdef was to be expected, imo a prereq change is to
> also make the struct fields conditional in xen/console.h. I think I did even
> comment to this effect back at the time.
Would you prefer that I include this change in the current patch
series, or is it acceptable to address it in a separate patch?
>
> Jan
Best Regards,
Mykola
On 24.06.2025 10:29, Mykola Kvach wrote:
> On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 24.06.2025 09:18, Mykola Kvach wrote:
>>> @@ -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
>>> };
>>
>> As this being put inside #ifdef was to be expected, imo a prereq change is to
>> also make the struct fields conditional in xen/console.h. I think I did even
>> comment to this effect back at the time.
>
> Would you prefer that I include this change in the current patch
> series, or is it acceptable to address it in a separate patch?
Either way is fine with me. I expect the header fine change to be able to go
in right away (once submitted), whereas the patch here may take some time for
people to review.
Jan
On Tue, Jun 24, 2025 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.06.2025 10:29, Mykola Kvach wrote:
> > On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 24.06.2025 09:18, Mykola Kvach wrote:
> >>> @@ -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
> >>> };
> >>
> >> As this being put inside #ifdef was to be expected, imo a prereq change is to
> >> also make the struct fields conditional in xen/console.h. I think I did even
> >> comment to this effect back at the time.
> >
> > Would you prefer that I include this change in the current patch
> > series, or is it acceptable to address it in a separate patch?
>
> Either way is fine with me. I expect the header fine change to be able to go
> in right away (once submitted), whereas the patch here may take some time for
> people to review.
Got it, I'll submit a separate patch to make the struct fields and
related code wrapped within SYSTEM_SUSPEND.
~Mykola
>
> Jan
Hi all,
On Tue, Jun 24, 2025 at 12:32 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>
> On Tue, Jun 24, 2025 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 24.06.2025 10:29, Mykola Kvach wrote:
> > > On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >> On 24.06.2025 09:18, Mykola Kvach wrote:
> > >>> @@ -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
> > >>> };
> > >>
> > >> As this being put inside #ifdef was to be expected, imo a prereq change is to
> > >> also make the struct fields conditional in xen/console.h. I think I did even
> > >> comment to this effect back at the time.
> > >
> > > Would you prefer that I include this change in the current patch
> > > series, or is it acceptable to address it in a separate patch?
> >
> > Either way is fine with me. I expect the header fine change to be able to go
> > in right away (once submitted), whereas the patch here may take some time for
> > people to review.
>
> Got it, I'll submit a separate patch to make the struct fields and
> related code wrapped within SYSTEM_SUSPEND.
>
> ~Mykola
>
> >
> > Jan
Jan’s proposal to conditionally include the 'suspend' and 'resume' fields
in 'struct uart_driver' under CONFIG_SYSTEM_SUSPEND has already been
merged -- thanks!
Could you please take another look at this patch when time permits?
Best regards,
Mykola
On 24.07.2025 13:41, Mykola Kvach wrote:
> Hi all,
>
> On Tue, Jun 24, 2025 at 12:32 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>>
>> On Tue, Jun 24, 2025 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 24.06.2025 10:29, Mykola Kvach wrote:
>>>> On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>> On 24.06.2025 09:18, Mykola Kvach wrote:
>>>>>> @@ -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
>>>>>> };
>>>>>
>>>>> As this being put inside #ifdef was to be expected, imo a prereq change is to
>>>>> also make the struct fields conditional in xen/console.h. I think I did even
>>>>> comment to this effect back at the time.
>>>>
>>>> Would you prefer that I include this change in the current patch
>>>> series, or is it acceptable to address it in a separate patch?
>>>
>>> Either way is fine with me. I expect the header fine change to be able to go
>>> in right away (once submitted), whereas the patch here may take some time for
>>> people to review.
>>
>> Got it, I'll submit a separate patch to make the struct fields and
>> related code wrapped within SYSTEM_SUSPEND.
>
> Jan’s proposal to conditionally include the 'suspend' and 'resume' fields
> in 'struct uart_driver' under CONFIG_SYSTEM_SUSPEND has already been
> merged -- thanks!
>
> Could you please take another look at this patch when time permits?
That's an Arm driver, so I don't expect the request was meant to go to me
(as To: having just me was suggesting)?
Jan
On Thu, Jul 24, 2025 at 3:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 24.07.2025 13:41, Mykola Kvach wrote:
> > Hi all,
> >
> > On Tue, Jun 24, 2025 at 12:32 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
> >>
> >> On Tue, Jun 24, 2025 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>
> >>> On 24.06.2025 10:29, Mykola Kvach wrote:
> >>>> On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> On 24.06.2025 09:18, Mykola Kvach wrote:
> >>>>>> @@ -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
> >>>>>> };
> >>>>>
> >>>>> As this being put inside #ifdef was to be expected, imo a prereq change is to
> >>>>> also make the struct fields conditional in xen/console.h. I think I did even
> >>>>> comment to this effect back at the time.
> >>>>
> >>>> Would you prefer that I include this change in the current patch
> >>>> series, or is it acceptable to address it in a separate patch?
> >>>
> >>> Either way is fine with me. I expect the header fine change to be able to go
> >>> in right away (once submitted), whereas the patch here may take some time for
> >>> people to review.
> >>
> >> Got it, I'll submit a separate patch to make the struct fields and
> >> related code wrapped within SYSTEM_SUSPEND.
> >
> > Jan’s proposal to conditionally include the 'suspend' and 'resume' fields
> > in 'struct uart_driver' under CONFIG_SYSTEM_SUSPEND has already been
> > merged -- thanks!
> >
> > Could you please take another look at this patch when time permits?
>
> That's an Arm driver, so I don't expect the request was meant to go to me
> (as To: having just me was suggesting)?
You're right -- this is an Arm driver, and I didn’t mean to direct
the request solely to you. Others in CC are also involved.
I thought the review of this patch had stalled following your
previous comment, so I just wanted to notify everyone involved
that the related changes have already been merged. With that out
of the way, I hope this remaining patch can now be reviewed and,
if acceptable, merged as well.
I’m not entirely familiar with the proper process for these cases,
so apologies if this ping was inappropriate or caused any
disruption.
>
> Jan
Best regards,
Mykola
On 24.07.2025 18:48, Mykola Kvach wrote:
> On Thu, Jul 24, 2025 at 3:13 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 24.07.2025 13:41, Mykola Kvach wrote:
>>> Hi all,
>>>
>>> On Tue, Jun 24, 2025 at 12:32 PM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>>>>
>>>> On Tue, Jun 24, 2025 at 11:32 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 24.06.2025 10:29, Mykola Kvach wrote:
>>>>>> On Tue, Jun 24, 2025 at 10:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>> On 24.06.2025 09:18, Mykola Kvach wrote:
>>>>>>>> @@ -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
>>>>>>>> };
>>>>>>>
>>>>>>> As this being put inside #ifdef was to be expected, imo a prereq change is to
>>>>>>> also make the struct fields conditional in xen/console.h. I think I did even
>>>>>>> comment to this effect back at the time.
>>>>>>
>>>>>> Would you prefer that I include this change in the current patch
>>>>>> series, or is it acceptable to address it in a separate patch?
>>>>>
>>>>> Either way is fine with me. I expect the header fine change to be able to go
>>>>> in right away (once submitted), whereas the patch here may take some time for
>>>>> people to review.
>>>>
>>>> Got it, I'll submit a separate patch to make the struct fields and
>>>> related code wrapped within SYSTEM_SUSPEND.
>>>
>>> Jan’s proposal to conditionally include the 'suspend' and 'resume' fields
>>> in 'struct uart_driver' under CONFIG_SYSTEM_SUSPEND has already been
>>> merged -- thanks!
>>>
>>> Could you please take another look at this patch when time permits?
>>
>> That's an Arm driver, so I don't expect the request was meant to go to me
>> (as To: having just me was suggesting)?
>
> You're right -- this is an Arm driver, and I didn’t mean to direct
> the request solely to you. Others in CC are also involved.
>
> I thought the review of this patch had stalled following your
> previous comment, so I just wanted to notify everyone involved
> that the related changes have already been merged. With that out
> of the way, I hope this remaining patch can now be reviewed and,
> if acceptable, merged as well.
>
> I’m not entirely familiar with the proper process for these cases,
> so apologies if this ping was inappropriate or caused any
> disruption.
It's not properly written down anywhere, afaik. My (personal) request
is that you make clear who you expect input from by properly arranging
To: vs Cc:.
Jan
© 2016 - 2025 Red Hat, Inc.