Hi, Geert, On 11.02.2025 10:16, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Fri, 7 Feb 2025 at 12:33, Claudiu <claudiu.beznea@xxxxxxxxx> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> >> >> 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 <claudiu.beznea.uj@xxxxxxxxxxxxxx> >> --- >> >> 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/20250120130936.1080069-1-claudiu.beznea.uj@xxxxxxxxxxxxxx >> >> 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 <geert+renesas@xxxxxxxxx> > > 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 -- geert@xxxxxxxxxxxxxx > > 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
