[v6] serial: sh-sci: Update the suspend/resume support

Commit Message

Claudiu Beznea Feb. 7, 2025, 11:33 a.m. UTC

From: Claudiu Beznea <[e-mail]>

The Renesas RZ/G3S supports a power saving mode where power to most of the
SoC components is turned off. When returning from this power saving mode,
SoC components need to be re-configured.

The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
returning from this power saving mode. The sh-sci code already configures
the SCIF clocks, power domain and registers by calling uart_resume_port()
in sci_resume(). On suspend path the SCIF UART ports are suspended
accordingly (by calling uart_suspend_port() in sci_suspend()). The only
missing setting is the reset signal. For this assert/de-assert the reset
signal on driver suspend/resume.

In case the no_console_suspend is specified by the user, the registers need
to be saved on suspend path and restore on resume path. To do this the
sci_console_save()/sci_console_restore() functions were added. There is no
need to cache/restore the status or FIFO registers. Only the control
registers. The registers that will be saved/restored on suspend/resume are
specified by the struct sci_suspend_regs data structure.

Signed-off-by: Claudiu Beznea <[e-mail]>
---

The v4 of this patch was part of a series with 4 patches. As the rest of
the patches were applied I dropped the cover letter. The v4 cover letter
is located here:
https://lore.kernel.org/all/[e-mail]

Changes in v6:
- used sci_getreg() before saving/restoring registers to avoid
  WARN() on sci_serial_in()/sci_serial_out()
- splitted sci_console_save_restore() in 2 functions:
  sci_console_save()/sci_console_restore() as requested in the
  review process
- adjusted the patch description to reflect these changes

Changes in v5:
- fixed typo in patch description
- adjusted the patch description to reflect the new patch content
- added struct sci_suspend_regs and dropped the suspend_cacheable
  flag introduced previously in struct plat_sci_reg; along with it
  the updates in sci_port_params[] were also dropped;
  also, the function (now sci_console_save_restore()) that saves
  and restores the registers content was adjusted accordingly
- s/sci_console_setup/sci_console_save_restore/g

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- rebased on top of the update version of patch 2/8 from
  this series

 drivers/tty/serial/sh-sci.c | 71 +++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Feb. 11, 2025, 8:16 a.m. UTC | #1

Hi Claudiu,

On Fri, 7 Feb 2025 at 12:33, Claudiu <[e-mail]> wrote:
> From: Claudiu Beznea <[e-mail]>
>
> The Renesas RZ/G3S supports a power saving mode where power to most of the
> SoC components is turned off. When returning from this power saving mode,
> SoC components need to be re-configured.
>
> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> returning from this power saving mode. The sh-sci code already configures
> the SCIF clocks, power domain and registers by calling uart_resume_port()
> in sci_resume(). On suspend path the SCIF UART ports are suspended
> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> missing setting is the reset signal. For this assert/de-assert the reset
> signal on driver suspend/resume.
>
> In case the no_console_suspend is specified by the user, the registers need
> to be saved on suspend path and restore on resume path. To do this the
> sci_console_save()/sci_console_restore() functions were added. There is no
> need to cache/restore the status or FIFO registers. Only the control
> registers. The registers that will be saved/restored on suspend/resume are
> specified by the struct sci_suspend_regs data structure.
>
> Signed-off-by: Claudiu Beznea <[e-mail]>
> ---
>
> The v4 of this patch was part of a series with 4 patches. As the rest of
> the patches were applied I dropped the cover letter. The v4 cover letter
> is located here:
> https://lore.kernel.org/all/[e-mail]
>
> Changes in v6:
> - used sci_getreg() before saving/restoring registers to avoid
>   WARN() on sci_serial_in()/sci_serial_out()
> - splitted sci_console_save_restore() in 2 functions:
>   sci_console_save()/sci_console_restore() as requested in the
>   review process
> - adjusted the patch description to reflect these changes

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <[e-mail]>

One philosophical comment below...

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3546,13 +3559,57 @@ static int sci_probe(struct platform_device *dev)
>         return 0;
>  }
>
> +static void sci_console_save(struct sci_port *s)
> +{
> +       struct sci_suspend_regs *regs = &s->suspend_regs;
> +       struct uart_port *port = &s->port;
> +
> +       if (sci_getreg(port, SCSMR)->size)
> +               regs->scsmr = sci_serial_in(port, SCSMR);
> +       if (sci_getreg(port, SCSCR)->size)
> +               regs->scscr = sci_serial_in(port, SCSCR);
> +       if (sci_getreg(port, SCFCR)->size)
> +               regs->scfcr = sci_serial_in(port, SCFCR);
> +       if (sci_getreg(port, SCSPTR)->size)
> +               regs->scsptr = sci_serial_in(port, SCSPTR);
> +       if (sci_getreg(port, SCBRR)->size)
> +               regs->scbrr = sci_serial_in(port, SCBRR);
> +       if (sci_getreg(port, SEMR)->size)
> +               regs->semr = sci_serial_in(port, SEMR);

IMHO, it does not make much sense to check for the presence of the
SCSMR, SCSCR, and SCBRR registers, as they exist on all variants,
and are always accessed unconditionally in the rest of the code.

Same for sci_console_restore().

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Claudiu Beznea Feb. 11, 2025, 10:51 a.m. UTC | #2

Hi, Geert,

On 11.02.2025 10:16, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, 7 Feb 2025 at 12:33, Claudiu <[e-mail]> wrote:
>> From: Claudiu Beznea <[e-mail]>
>>
>> The Renesas RZ/G3S supports a power saving mode where power to most of the
>> SoC components is turned off. When returning from this power saving mode,
>> SoC components need to be re-configured.
>>
>> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
>> returning from this power saving mode. The sh-sci code already configures
>> the SCIF clocks, power domain and registers by calling uart_resume_port()
>> in sci_resume(). On suspend path the SCIF UART ports are suspended
>> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
>> missing setting is the reset signal. For this assert/de-assert the reset
>> signal on driver suspend/resume.
>>
>> In case the no_console_suspend is specified by the user, the registers need
>> to be saved on suspend path and restore on resume path. To do this the
>> sci_console_save()/sci_console_restore() functions were added. There is no
>> need to cache/restore the status or FIFO registers. Only the control
>> registers. The registers that will be saved/restored on suspend/resume are
>> specified by the struct sci_suspend_regs data structure.
>>
>> Signed-off-by: Claudiu Beznea <[e-mail]>
>> ---
>>
>> The v4 of this patch was part of a series with 4 patches. As the rest of
>> the patches were applied I dropped the cover letter. The v4 cover letter
>> is located here:
>> https://lore.kernel.org/all/[e-mail]
>>
>> Changes in v6:
>> - used sci_getreg() before saving/restoring registers to avoid
>>   WARN() on sci_serial_in()/sci_serial_out()
>> - splitted sci_console_save_restore() in 2 functions:
>>   sci_console_save()/sci_console_restore() as requested in the
>>   review process
>> - adjusted the patch description to reflect these changes
> 
> Thanks for the update!
> 
> Reviewed-by: Geert Uytterhoeven <[e-mail]>
> 
> One philosophical comment below...
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3546,13 +3559,57 @@ static int sci_probe(struct platform_device *dev)
>>         return 0;
>>  }
>>
>> +static void sci_console_save(struct sci_port *s)
>> +{
>> +       struct sci_suspend_regs *regs = &s->suspend_regs;
>> +       struct uart_port *port = &s->port;
>> +
>> +       if (sci_getreg(port, SCSMR)->size)
>> +               regs->scsmr = sci_serial_in(port, SCSMR);
>> +       if (sci_getreg(port, SCSCR)->size)
>> +               regs->scscr = sci_serial_in(port, SCSCR);
>> +       if (sci_getreg(port, SCFCR)->size)
>> +               regs->scfcr = sci_serial_in(port, SCFCR);
>> +       if (sci_getreg(port, SCSPTR)->size)
>> +               regs->scsptr = sci_serial_in(port, SCSPTR);
>> +       if (sci_getreg(port, SCBRR)->size)
>> +               regs->scbrr = sci_serial_in(port, SCBRR);
>> +       if (sci_getreg(port, SEMR)->size)
>> +               regs->semr = sci_serial_in(port, SEMR);
> 
> IMHO, it does not make much sense to check for the presence of the
> SCSMR, SCSCR, and SCBRR registers, as they exist on all variants,
> and are always accessed unconditionally in the rest of the code.

I kept it like this thinking that it may help keeping this common for
RZ/T2H (though I confess I haven't checked it) and avoid future WARN() on
other possible platforms. If you prefer, I can drop the checks you pointed.

Thank you,
Claudiu

> 
> Same for sci_console_restore().
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index b1ea48f38248..4bc637f9d649 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -104,6 +104,15 @@  struct plat_sci_reg {
 	u8 offset, size;
 };
 
+struct sci_suspend_regs {
+	u16 scsmr;
+	u16 scscr;
+	u16 scfcr;
+	u16 scsptr;
+	u8 scbrr;
+	u8 semr;
+};
+
 struct sci_port_params {
 	const struct plat_sci_reg regs[SCIx_NR_REGS];
 	unsigned int fifosize;
@@ -134,6 +143,8 @@  struct sci_port {
 	struct dma_chan			*chan_tx;
 	struct dma_chan			*chan_rx;
 
+	struct reset_control		*rstc;
+
 #ifdef CONFIG_SERIAL_SH_SCI_DMA
 	struct dma_chan			*chan_tx_saved;
 	struct dma_chan			*chan_rx_saved;
@@ -153,6 +164,7 @@  struct sci_port {
 	int				rx_trigger;
 	struct timer_list		rx_fifo_timer;
 	int				rx_fifo_timeout;
+	struct sci_suspend_regs		suspend_regs;
 	u16				hscif_tot;
 
 	bool has_rtscts;
@@ -3374,6 +3386,7 @@  static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 	}
 
 	sp = &sci_ports[id];
+	sp->rstc = rstc;
 	*dev_id = id;
 
 	p->type = SCI_OF_TYPE(data);
@@ -3546,13 +3559,57 @@  static int sci_probe(struct platform_device *dev)
 	return 0;
 }
 
+static void sci_console_save(struct sci_port *s)
+{
+	struct sci_suspend_regs *regs = &s->suspend_regs;
+	struct uart_port *port = &s->port;
+
+	if (sci_getreg(port, SCSMR)->size)
+		regs->scsmr = sci_serial_in(port, SCSMR);
+	if (sci_getreg(port, SCSCR)->size)
+		regs->scscr = sci_serial_in(port, SCSCR);
+	if (sci_getreg(port, SCFCR)->size)
+		regs->scfcr = sci_serial_in(port, SCFCR);
+	if (sci_getreg(port, SCSPTR)->size)
+		regs->scsptr = sci_serial_in(port, SCSPTR);
+	if (sci_getreg(port, SCBRR)->size)
+		regs->scbrr = sci_serial_in(port, SCBRR);
+	if (sci_getreg(port, SEMR)->size)
+		regs->semr = sci_serial_in(port, SEMR);
+}
+
+static void sci_console_restore(struct sci_port *s)
+{
+	struct sci_suspend_regs *regs = &s->suspend_regs;
+	struct uart_port *port = &s->port;
+
+	if (sci_getreg(port, SCSMR)->size)
+		sci_serial_out(port, SCSMR, regs->scsmr);
+	if (sci_getreg(port, SCSCR)->size)
+		sci_serial_out(port, SCSCR, regs->scscr);
+	if (sci_getreg(port, SCFCR)->size)
+		sci_serial_out(port, SCFCR, regs->scfcr);
+	if (sci_getreg(port, SCSPTR)->size)
+		sci_serial_out(port, SCSPTR, regs->scsptr);
+	if (sci_getreg(port, SCBRR)->size)
+		sci_serial_out(port, SCBRR, regs->scbrr);
+	if (sci_getreg(port, SEMR)->size)
+		sci_serial_out(port, SEMR, regs->semr);
+}
+
 static __maybe_unused int sci_suspend(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
 		uart_suspend_port(&sci_uart_driver, &sport->port);
 
+		if (!console_suspend_enabled && uart_console(&sport->port))
+			sci_console_save(sport);
+		else
+			return reset_control_assert(sport->rstc);
+	}
+
 	return 0;
 }
 
@@ -3560,8 +3617,18 @@  static __maybe_unused int sci_resume(struct device *dev)
 {
 	struct sci_port *sport = dev_get_drvdata(dev);
 
-	if (sport)
+	if (sport) {
+		if (!console_suspend_enabled && uart_console(&sport->port)) {
+			sci_console_restore(sport);
+		} else {
+			int ret = reset_control_deassert(sport->rstc);
+
+			if (ret)
+				return ret;
+		}
+
 		uart_resume_port(&sci_uart_driver, &sport->port);
+	}
 
 	return 0;
 }