| Message ID | 800acb5c8447153e6c451c8df316ff678fbb4087.1743114879.git.mykola_kvach@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/char: implement suspend/resume calls for SCIF driver | expand |
On 28/03/2025 08:08, Mykola Kvach wrote: > > > From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > The changes have been tested only on the Renesas R-Car H3 Starter Kit board. > > 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> I'm afraid that without a suspend feature on Arm merged, this is just introducing a dead code which we try to eliminate both for MISRA and safety. I'd prefer to wait for the suspend feature to be merged first. ~Michal
Hi Michal, On 28/03/2025 08:39, Orzel, Michal wrote: > > > On 28/03/2025 08:08, Mykola Kvach wrote: >> >> >> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> >> The changes have been tested only on the Renesas R-Car H3 Starter Kit board. >> >> 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> > I'm afraid that without a suspend feature on Arm merged, this is just > introducing a dead code which we try to eliminate both for MISRA and safety. > I'd prefer to wait for the suspend feature to be merged first. This patch is not different from the ns16550 driver which already have suspend/resume callback and users. They will be used by but the caller is not used on Arm yet. So you are saying the code there is not MISRA compliant? If so, is this reported by ECLAIR? Regardless that, I don't think the full suspend/resume series would help. AFAIR, the caller will be protected with a config (SUSPEND something). So IIUC your definition, this code will still be "dead code" in certain config. Therefore are you suggesting to protect everything suspend specific code with SUSPEND? If the answer is yes, how about introducing the SUSPEND config now? This would allow to get some of the code merged in advance. Cheers,
On 28/03/2025 09:57, Julien Grall wrote: > > > Hi Michal, > > On 28/03/2025 08:39, Orzel, Michal wrote: >> >> >> On 28/03/2025 08:08, Mykola Kvach wrote: >>> >>> >>> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>> >>> The changes have been tested only on the Renesas R-Car H3 Starter Kit board. >>> >>> 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> >> I'm afraid that without a suspend feature on Arm merged, this is just >> introducing a dead code which we try to eliminate both for MISRA and safety. >> I'd prefer to wait for the suspend feature to be merged first. > > This patch is not different from the ns16550 driver which already have > suspend/resume callback and users. They will be used by but the caller > is not used on Arm yet. So you are saying the code there is not MISRA > compliant? If so, is this reported by ECLAIR? NS also works for x86 that has suspend feature. SCIF only works for Arm that does not have suspend feature. Neither NS nor SCIF are enabled in ECLAIR. We have enabled MISRA 2.1 rule about unreachable code (suspend is example of unreachable because it *cannot* be executed), but it's not clean i.e. gating. > > Regardless that, I don't think the full suspend/resume series would > help. AFAIR, the caller will be protected with a config (SUSPEND > something). So IIUC your definition, this code will still be "dead code" > in certain config. Therefore are you suggesting to protect everything > suspend specific code with SUSPEND? I'd say so, yes. > > If the answer is yes, how about introducing the SUSPEND config now? This > would allow to get some of the code merged in advance. Better this than nothing. But in case of this patch, why would we take it anyway without suspend feature? I'd prefer to add SUSPEND config and protect existing code i.e. NS that can work on x86 but not on Arm, etc. ~Michal
On 28/03/2025 09:13, Orzel, Michal wrote: > > > On 28/03/2025 09:57, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 28/03/2025 08:39, Orzel, Michal wrote: >>> >>> >>> On 28/03/2025 08:08, Mykola Kvach wrote: >>>> >>>> >>>> From: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >>>> >>>> The changes have been tested only on the Renesas R-Car H3 Starter Kit board. >>>> >>>> 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> >>> I'm afraid that without a suspend feature on Arm merged, this is just >>> introducing a dead code which we try to eliminate both for MISRA and safety. >>> I'd prefer to wait for the suspend feature to be merged first. >> >> This patch is not different from the ns16550 driver which already have >> suspend/resume callback and users. They will be used by but the caller >> is not used on Arm yet. So you are saying the code there is not MISRA >> compliant? If so, is this reported by ECLAIR? > NS also works for x86 that has suspend feature. SCIF only works for Arm that > does not have suspend feature. I don't understand why it matters that NS16550 is used on x86. In previous MISRA discussion we said the violations were tracked per arch rather than globally. [...] >> >> Regardless that, I don't think the full suspend/resume series would >> help. AFAIR, the caller will be protected with a config (SUSPEND >> something). So IIUC your definition, this code will still be "dead code" >> in certain config. Therefore are you suggesting to protect everything >> suspend specific code with SUSPEND? > I'd say so, yes. > >> >> If the answer is yes, how about introducing the SUSPEND config now? This >> would allow to get some of the code merged in advance. > Better this than nothing. But in case of this patch, why would we take it anyway > without suspend feature? Two reasons: * There is nothing really preventing the SCIF to be used on x86 (yes I know there are only Arm HW today) * We will likely want to commit the series bits by bits to avoid dozen of patches to be sent everytime. This patch is something small enough and can be committed before hand. > I'd prefer to add SUSPEND config and protect existing > code i.e. NS that can work on x86 but not on Arm, etc. I am fine if we want to add the SUSPEND config first. Cheers,
diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c index 757793ca45..ce0f87c650 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,27 @@ static void scif_uart_stop_tx(struct serial_port *port) scif_writew(uart, SCIF_SCSCR, scif_readw(uart, SCIF_SCSCR) & ~SCSCR_TIE); } +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); +} + static struct uart_driver __read_mostly scif_uart_driver = { .init_preirq = scif_uart_init_preirq, .init_postirq = scif_uart_init_postirq, @@ -281,6 +309,8 @@ 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, + .suspend = scif_uart_suspend, + .resume = scif_uart_resume, }; static const struct dt_device_match scif_uart_dt_match[] __initconst =