| Message ID | 20241115134401.3893008-4-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add support for the rest of Renesas RZ/G3S serial interfaces | expand |
On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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_setup() function was added. There is no need to cache/restore > the status or FIFO registers. Only the control registers. To differentiate > b/w these, the struct sci_port_params::regs was updated with a new member > that specifies if the register needs to be chached on suspend. Only the > RZ_SCIFA instances were updated with this new support as the hardware for > the rest of variants was missing for testing. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > 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 | 53 ++++++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 9 deletions(-) > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > index ade151ff39d2..e53496d2708e 100644 > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -101,7 +101,7 @@ enum SCI_CLKS { > if ((_port)->sampling_rate_mask & SCI_SR((_sr))) > > struct plat_sci_reg { > - u8 offset, size; > + u8 offset, size, suspend_cacheable; > }; > > struct sci_port_params { > @@ -134,6 +134,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 +155,7 @@ struct sci_port { > int rx_trigger; > struct timer_list rx_fifo_timer; > int rx_fifo_timeout; > + unsigned int console_cached_regs[SCIx_NR_REGS]; > u16 hscif_tot; > > bool has_rtscts; > @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { > */ > [SCIx_RZ_SCIFA_REGTYPE] = { > .regs = { > - [SCSMR] = { 0x00, 16 }, > - [SCBRR] = { 0x02, 8 }, > - [SCSCR] = { 0x04, 16 }, > + [SCSMR] = { 0x00, 16, 1 }, > + [SCBRR] = { 0x02, 8, 1 }, > + [SCSCR] = { 0x04, 16, 1 }, > [SCxTDR] = { 0x06, 8 }, > [SCxSR] = { 0x08, 16 }, > [SCxRDR] = { 0x0A, 8 }, > - [SCFCR] = { 0x0C, 16 }, > + [SCFCR] = { 0x0C, 16, 1 }, > [SCFDR] = { 0x0E, 16 }, > - [SCSPTR] = { 0x10, 16 }, > + [SCSPTR] = { 0x10, 16, 1 }, > [SCLSR] = { 0x12, 16 }, > - [SEMR] = { 0x14, 8 }, > + [SEMR] = { 0x14, 8, 1 }, > }, > .fifosize = 16, > .overrun_reg = SCLSR, > @@ -3380,6 +3383,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); > @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev) > return 0; > } > > +static void sci_console_setup(struct sci_port *s, bool save) > +{ > + for (u16 i = 0; i < SCIx_NR_REGS; i++) { > + struct uart_port *port = &s->port; > + > + if (!s->params->regs[i].suspend_cacheable) > + continue; > + > + if (save) > + s->console_cached_regs[i] = sci_serial_in(port, i); > + else > + sci_serial_out(port, i, s->console_cached_regs[i]); > + } > +} > + > 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_setup(sport, true); > + else > + return reset_control_assert(sport->rstc); > + } > + > return 0; > } > > @@ -3521,8 +3546,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_setup(sport, false); > + } else { > + int ret = reset_control_deassert(sport->rstc); With this, is the reset_control_deassert() in sci_parse_dt() still needed? Likewise, does the reset_control_assert() in sci_suspend() remove the need for the sci_reset_control_assert() devm action? regards Philipp
Hi, Philipp, On 15.11.2024 17:40, Philipp Zabel wrote: > On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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_setup() function was added. There is no need to cache/restore >> the status or FIFO registers. Only the control registers. To differentiate >> b/w these, the struct sci_port_params::regs was updated with a new member >> that specifies if the register needs to be chached on suspend. Only the >> RZ_SCIFA instances were updated with this new support as the hardware for >> the rest of variants was missing for testing. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> 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 | 53 ++++++++++++++++++++++++++++++------- >> 1 file changed, 44 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c >> index ade151ff39d2..e53496d2708e 100644 >> --- a/drivers/tty/serial/sh-sci.c >> +++ b/drivers/tty/serial/sh-sci.c >> @@ -101,7 +101,7 @@ enum SCI_CLKS { >> if ((_port)->sampling_rate_mask & SCI_SR((_sr))) >> >> struct plat_sci_reg { >> - u8 offset, size; >> + u8 offset, size, suspend_cacheable; >> }; >> >> struct sci_port_params { >> @@ -134,6 +134,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 +155,7 @@ struct sci_port { >> int rx_trigger; >> struct timer_list rx_fifo_timer; >> int rx_fifo_timeout; >> + unsigned int console_cached_regs[SCIx_NR_REGS]; >> u16 hscif_tot; >> >> bool has_rtscts; >> @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { >> */ >> [SCIx_RZ_SCIFA_REGTYPE] = { >> .regs = { >> - [SCSMR] = { 0x00, 16 }, >> - [SCBRR] = { 0x02, 8 }, >> - [SCSCR] = { 0x04, 16 }, >> + [SCSMR] = { 0x00, 16, 1 }, >> + [SCBRR] = { 0x02, 8, 1 }, >> + [SCSCR] = { 0x04, 16, 1 }, >> [SCxTDR] = { 0x06, 8 }, >> [SCxSR] = { 0x08, 16 }, >> [SCxRDR] = { 0x0A, 8 }, >> - [SCFCR] = { 0x0C, 16 }, >> + [SCFCR] = { 0x0C, 16, 1 }, >> [SCFDR] = { 0x0E, 16 }, >> - [SCSPTR] = { 0x10, 16 }, >> + [SCSPTR] = { 0x10, 16, 1 }, >> [SCLSR] = { 0x12, 16 }, >> - [SEMR] = { 0x14, 8 }, >> + [SEMR] = { 0x14, 8, 1 }, >> }, >> .fifosize = 16, >> .overrun_reg = SCLSR, >> @@ -3380,6 +3383,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); >> @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev) >> return 0; >> } >> >> +static void sci_console_setup(struct sci_port *s, bool save) >> +{ >> + for (u16 i = 0; i < SCIx_NR_REGS; i++) { >> + struct uart_port *port = &s->port; >> + >> + if (!s->params->regs[i].suspend_cacheable) >> + continue; >> + >> + if (save) >> + s->console_cached_regs[i] = sci_serial_in(port, i); >> + else >> + sci_serial_out(port, i, s->console_cached_regs[i]); >> + } >> +} >> + >> 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_setup(sport, true); >> + else >> + return reset_control_assert(sport->rstc); >> + } >> + >> return 0; >> } >> >> @@ -3521,8 +3546,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_setup(sport, false); >> + } else { >> + int ret = reset_control_deassert(sport->rstc); > > With this, is the reset_control_deassert() in sci_parse_dt() still > needed? If I'm not wrongly understanding your question, yes, the reset_control_deassert() is still needed in the sci_parse_dt() as the sci_parse_dt() is called on probe path. After resume the sci_parse_dt() is not called unless the driver is unbinded and then re-binded. In case the reset_control_dessert() here fails (or not) and an unbind/re-bind will be requested, the unbind will call reset_control_assert() (though the devm action) and then the re-bind will call reset_control_deassert() though sci_parse_dt(). That should be safe, AFAICT. > > Likewise, does the reset_control_assert() in sci_suspend() remove the > need for the sci_reset_control_assert() devm action? No, the sci_reset_control_assert() is still needed as explained above, unless I missed your point. Please let me know if missed your point and/or answered your question? Thank you, Claudiu Beznea > > regards > Philipp
On Mo, 2024-11-18 at 11:47 +0200, Claudiu Beznea wrote: > Hi, Philipp, > > On 15.11.2024 17:40, Philipp Zabel wrote: > > On Fr, 2024-11-15 at 15:43 +0200, Claudiu wrote: > > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > > > 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_setup() function was added. There is no need to cache/restore > > > the status or FIFO registers. Only the control registers. To differentiate > > > b/w these, the struct sci_port_params::regs was updated with a new member > > > that specifies if the register needs to be chached on suspend. Only the > > > RZ_SCIFA instances were updated with this new support as the hardware for > > > the rest of variants was missing for testing. > > > > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > --- > > > > > > 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 | 53 ++++++++++++++++++++++++++++++------- > > > 1 file changed, 44 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c > > > index ade151ff39d2..e53496d2708e 100644 > > > --- a/drivers/tty/serial/sh-sci.c > > > +++ b/drivers/tty/serial/sh-sci.c > > > @@ -101,7 +101,7 @@ enum SCI_CLKS { > > > if ((_port)->sampling_rate_mask & SCI_SR((_sr))) > > > > > > struct plat_sci_reg { > > > - u8 offset, size; > > > + u8 offset, size, suspend_cacheable; > > > }; > > > > > > struct sci_port_params { > > > @@ -134,6 +134,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 +155,7 @@ struct sci_port { > > > int rx_trigger; > > > struct timer_list rx_fifo_timer; > > > int rx_fifo_timeout; > > > + unsigned int console_cached_regs[SCIx_NR_REGS]; > > > u16 hscif_tot; > > > > > > bool has_rtscts; > > > @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { > > > */ > > > [SCIx_RZ_SCIFA_REGTYPE] = { > > > .regs = { > > > - [SCSMR] = { 0x00, 16 }, > > > - [SCBRR] = { 0x02, 8 }, > > > - [SCSCR] = { 0x04, 16 }, > > > + [SCSMR] = { 0x00, 16, 1 }, > > > + [SCBRR] = { 0x02, 8, 1 }, > > > + [SCSCR] = { 0x04, 16, 1 }, > > > [SCxTDR] = { 0x06, 8 }, > > > [SCxSR] = { 0x08, 16 }, > > > [SCxRDR] = { 0x0A, 8 }, > > > - [SCFCR] = { 0x0C, 16 }, > > > + [SCFCR] = { 0x0C, 16, 1 }, > > > [SCFDR] = { 0x0E, 16 }, > > > - [SCSPTR] = { 0x10, 16 }, > > > + [SCSPTR] = { 0x10, 16, 1 }, > > > [SCLSR] = { 0x12, 16 }, > > > - [SEMR] = { 0x14, 8 }, > > > + [SEMR] = { 0x14, 8, 1 }, > > > }, > > > .fifosize = 16, > > > .overrun_reg = SCLSR, > > > @@ -3380,6 +3383,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); > > > @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev) > > > return 0; > > > } > > > > > > +static void sci_console_setup(struct sci_port *s, bool save) > > > +{ > > > + for (u16 i = 0; i < SCIx_NR_REGS; i++) { > > > + struct uart_port *port = &s->port; > > > + > > > + if (!s->params->regs[i].suspend_cacheable) > > > + continue; > > > + > > > + if (save) > > > + s->console_cached_regs[i] = sci_serial_in(port, i); > > > + else > > > + sci_serial_out(port, i, s->console_cached_regs[i]); > > > + } > > > +} > > > + > > > 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_setup(sport, true); > > > + else > > > + return reset_control_assert(sport->rstc); > > > + } > > > + > > > return 0; > > > } > > > > > > @@ -3521,8 +3546,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_setup(sport, false); > > > + } else { > > > + int ret = reset_control_deassert(sport->rstc); > > > > With this, is the reset_control_deassert() in sci_parse_dt() still > > needed? > > If I'm not wrongly understanding your question, yes, the > reset_control_deassert() is still needed in the sci_parse_dt() as the > sci_parse_dt() is called on probe path. After resume the sci_parse_dt() is > not called unless the driver is unbinded and then re-binded. Ah, I was thinking of runtime PM resume callbacks. Thank you for the answer, this cleared my confusion. regards Philipp
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index ade151ff39d2..e53496d2708e 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -101,7 +101,7 @@ enum SCI_CLKS { if ((_port)->sampling_rate_mask & SCI_SR((_sr))) struct plat_sci_reg { - u8 offset, size; + u8 offset, size, suspend_cacheable; }; struct sci_port_params { @@ -134,6 +134,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 +155,7 @@ struct sci_port { int rx_trigger; struct timer_list rx_fifo_timer; int rx_fifo_timeout; + unsigned int console_cached_regs[SCIx_NR_REGS]; u16 hscif_tot; bool has_rtscts; @@ -298,17 +301,17 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { */ [SCIx_RZ_SCIFA_REGTYPE] = { .regs = { - [SCSMR] = { 0x00, 16 }, - [SCBRR] = { 0x02, 8 }, - [SCSCR] = { 0x04, 16 }, + [SCSMR] = { 0x00, 16, 1 }, + [SCBRR] = { 0x02, 8, 1 }, + [SCSCR] = { 0x04, 16, 1 }, [SCxTDR] = { 0x06, 8 }, [SCxSR] = { 0x08, 16 }, [SCxRDR] = { 0x0A, 8 }, - [SCFCR] = { 0x0C, 16 }, + [SCFCR] = { 0x0C, 16, 1 }, [SCFDR] = { 0x0E, 16 }, - [SCSPTR] = { 0x10, 16 }, + [SCSPTR] = { 0x10, 16, 1 }, [SCLSR] = { 0x12, 16 }, - [SEMR] = { 0x14, 8 }, + [SEMR] = { 0x14, 8, 1 }, }, .fifosize = 16, .overrun_reg = SCLSR, @@ -3380,6 +3383,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); @@ -3507,13 +3511,34 @@ static int sci_probe(struct platform_device *dev) return 0; } +static void sci_console_setup(struct sci_port *s, bool save) +{ + for (u16 i = 0; i < SCIx_NR_REGS; i++) { + struct uart_port *port = &s->port; + + if (!s->params->regs[i].suspend_cacheable) + continue; + + if (save) + s->console_cached_regs[i] = sci_serial_in(port, i); + else + sci_serial_out(port, i, s->console_cached_regs[i]); + } +} + 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_setup(sport, true); + else + return reset_control_assert(sport->rstc); + } + return 0; } @@ -3521,8 +3546,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_setup(sport, false); + } else { + int ret = reset_control_deassert(sport->rstc); + + if (ret) + return ret; + } + uart_resume_port(&sci_uart_driver, &sport->port); + } return 0; }