| Message ID | 20250120130936.1080069-2-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 20/01/2025 14:09, 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 v4: > - none Why are you combining serial patches with DTS? Greg applies entire set thus you *cannot* send him DTS. Best regards, Krzysztof
On 21.01.2025 10:54, Krzysztof Kozlowski wrote: > On 20/01/2025 14:09, 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 v4: >> - none > > > Why are you combining serial patches with DTS? Greg applies entire set > thus you *cannot* send him DTS. It's v4. The initial set contained fixes for serial, support for RZ/G3S (including clocks and dtsi), all that was needed for the enabled RZ/G3S serial IPs. Fixes were posted separately (as requested), the other bringup patches were integrated and this is what remained. I chose it like this for version continuity. Thank you, Claudiu > > Best regards, > Krzysztof
Hi Claudiu, Thanks for your patch! On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> 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 cached > 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> > --- 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; This increases the size of sci_port_params[] by 300 bytes. Using bitfields would mitigate that: struct plat_sci_reg { u16 offset:8; u16 size:5; u16 suspend_cacheable:1; }; (if we ever need more bits, the size member can store an enum value instead of the actual size (8 or 16 bits) of the register). > }; > > 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, as all registers are 8 or 16 bit wide. We reserve space for 20 registers, but at most 6 will be used. This has a rather big impact on the size of sci_ports[], as CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18. Also, this space is used/needed only if: - CONFIG_PM_SLEEP=y, - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()), - The port is actually used as a console (unfortunately the user can specify multiple console=ttySC<N> command line parameters, in addition to chosen/stdout-path). > u16 hscif_tot; > > bool has_rtscts; > @@ -300,17 +303,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 }, Note that the driver always writes zero to SEMR. > }, > .fifosize = 16, > .overrun_reg = SCLSR, > @@ -3374,6 +3377,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 +3550,34 @@ static int sci_probe(struct platform_device *dev) > return 0; > } > > +static void sci_console_setup(struct sci_port *s, bool save) sci_console_save_restore()? > +{ > + for (u16 i = 0; i < SCIx_NR_REGS; i++) { unsigned int > + 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]); > + } > +} Gr{oetje,eeting}s, Geert
Hi, Geert, On 24.01.2025 12:53, Geert Uytterhoeven wrote: > Hi Claudiu, > > Thanks for your patch! > > On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> 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 > > cached > >> 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> > >> --- 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; > > This increases the size of sci_port_params[] by 300 bytes. > Using bitfields would mitigate that: > > struct plat_sci_reg { > u16 offset:8; > u16 size:5; > u16 suspend_cacheable:1; > }; > > (if we ever need more bits, the size member can store an enum value > instead of the actual size (8 or 16 bits) of the register). > >> }; OK >> >> 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, as all registers are 8 or 16 bit wide. OK. > > We reserve space for 20 registers, but at most 6 will be used. > This has a rather big impact on the size of sci_ports[], as > CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18. I agree, but this should keep the suspend/resume code sane in case extensions will be added to the code. In general people forget about suspend/resume code when extending. Please let me know if you prefer to limit it (although, doing like this will complicate the code, I think). > > Also, this space is used/needed only if: > - CONFIG_PM_SLEEP=y, > - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()), > - The port is actually used as a console (unfortunately the user > can specify multiple console=ttySC<N> command line parameters, in > addition to chosen/stdout-path). Would you prefer to guard the suspend/resume code with these flags? > >> u16 hscif_tot; >> >> bool has_rtscts; >> @@ -300,17 +303,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 }, > > Note that the driver always writes zero to SEMR. In case the IP is used on SoCs with sleep states where the resume is done with the help of bootloader, the bootloader code might interact with registers that the Linux code writes with zero. Keeping it for registers where driver writes zero should also help if the serial IPs power will be off during suspend, thus registers restored to non zero default values (by HW) after resume. > >> }, >> .fifosize = 16, >> .overrun_reg = SCLSR, >> @@ -3374,6 +3377,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 +3550,34 @@ static int sci_probe(struct platform_device *dev) >> return 0; >> } >> >> +static void sci_console_setup(struct sci_port *s, bool save) > > sci_console_save_restore()? OK > >> +{ >> + for (u16 i = 0; i < SCIx_NR_REGS; i++) { > > unsigned int OK > >> + 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]); >> + } >> +} > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > On 24.01.2025 12:53, Geert Uytterhoeven wrote: > > On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> 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 > > > > cached > > > >> 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> > > > >> --- 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; > > > > This increases the size of sci_port_params[] by 300 bytes. > > Using bitfields would mitigate that: > > > > struct plat_sci_reg { > > u16 offset:8; > > u16 size:5; > > u16 suspend_cacheable:1; > > }; > > > > (if we ever need more bits, the size member can store an enum value > > instead of the actual size (8 or 16 bits) of the register). > > > >> }; > > OK > > >> > >> 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, as all registers are 8 or 16 bit wide. > > OK. > > > > > We reserve space for 20 registers, but at most 6 will be used. > > This has a rather big impact on the size of sci_ports[], as > > CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18. > > I agree, but this should keep the suspend/resume code sane in case > extensions will be added to the code. In general people forget about > suspend/resume code when extending. Please let me know if you prefer to > limit it (although, doing like this will complicate the code, I think). > > > > > Also, this space is used/needed only if: > > - CONFIG_PM_SLEEP=y, > > - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()), > > - The port is actually used as a console (unfortunately the user > > can specify multiple console=ttySC<N> command line parameters, in > > addition to chosen/stdout-path). > > Would you prefer to guard the suspend/resume code with these flags? I was also thinking about console_cached_regs[]. But if you would protect that by #ifdef, you also have to protect the code that uses it, meaning less compile coverage. If you just add a static inline helper function to check for CONFIG_PM_SLEEP, !console_suspend_enabled, and uart_console(&sport->port): static bool sci_console_keep_alive(struct sci_port *sport) { return IS_ENABLED(CONFIG_PM_SLEEP) && !console_suspend_enabled && uart_console(&sport->port); } then most of the code will be validated but optimized away when unused. > >> u16 hscif_tot; > >> > >> bool has_rtscts; > >> @@ -300,17 +303,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 }, > > > > Note that the driver always writes zero to SEMR. > > In case the IP is used on SoCs with sleep states where the resume is done > with the help of bootloader, the bootloader code might interact with > registers that the Linux code writes with zero. > > Keeping it for registers where driver writes zero should also help if the > serial IPs power will be off during suspend, thus registers restored to non > zero default values (by HW) after resume. Sure, the driver would have to write zero to the register anyway. While storing the suspend_cacheable flag wouldn't cost any storage space anymore using bitfields, I am wondering if it would be worthwhile to have explicit code to save/restore registers, instead of looping over all of them and checking the flag. I.e. u16 saved_scmsr; u16 saved_scscr; u8 saved_scbrr; ... u8 saved_semr; /* Save omnipresent registers */ s->saved_scmsr = sci_serial_in(port, SCSMR); ... /* Save optional registers */ if (sci_getreg(port, SEMR)->size) s->saved_semr = sci_serial_in(port, SEMR); That would make it apply to all SCI variants, not just for SCIFA, while increasing sci_port by only 10 bytes/port. And 10 bytes/port is probably not worth to be protected by an #ifdef... Thoughts? Gr{oetje,eeting}s, Geert
Hi, Geert, On 27.01.2025 11:19, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: >> On 24.01.2025 12:53, Geert Uytterhoeven wrote: >>> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> 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 >>> >>> cached >>> >>>> 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> >>> >>>> --- 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; >>> >>> This increases the size of sci_port_params[] by 300 bytes. >>> Using bitfields would mitigate that: >>> >>> struct plat_sci_reg { >>> u16 offset:8; >>> u16 size:5; >>> u16 suspend_cacheable:1; >>> }; >>> >>> (if we ever need more bits, the size member can store an enum value >>> instead of the actual size (8 or 16 bits) of the register). >>> >>>> }; >> >> OK >> >>>> >>>> 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, as all registers are 8 or 16 bit wide. >> >> OK. >> >>> >>> We reserve space for 20 registers, but at most 6 will be used. >>> This has a rather big impact on the size of sci_ports[], as >>> CONFIG_SERIAL_SH_SCI_NR_UARTS defaults to 18. >> >> I agree, but this should keep the suspend/resume code sane in case >> extensions will be added to the code. In general people forget about >> suspend/resume code when extending. Please let me know if you prefer to >> limit it (although, doing like this will complicate the code, I think). >> >>> >>> Also, this space is used/needed only if: >>> - CONFIG_PM_SLEEP=y, >>> - CONFIG_SERIAL_CORE_CONSOLE=y (see uart_console()), >>> - The port is actually used as a console (unfortunately the user >>> can specify multiple console=ttySC<N> command line parameters, in >>> addition to chosen/stdout-path). >> >> Would you prefer to guard the suspend/resume code with these flags? > > I was also thinking about console_cached_regs[]. Agree. > But if you would > protect that by #ifdef, you also have to protect the code that uses it, > meaning less compile coverage. > > If you just add a static inline helper function to check for > CONFIG_PM_SLEEP, !console_suspend_enabled, and > uart_console(&sport->port): > > static bool sci_console_keep_alive(struct sci_port *sport) > { > return IS_ENABLED(CONFIG_PM_SLEEP) && > !console_suspend_enabled && uart_console(&sport->port); > } > > then most of the code will be validated but optimized away when unused. I wasn't aware of this approach. I'll give it a try, if any. > >>>> u16 hscif_tot; >>>> >>>> bool has_rtscts; >>>> @@ -300,17 +303,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 }, >>> >>> Note that the driver always writes zero to SEMR. >> >> In case the IP is used on SoCs with sleep states where the resume is done >> with the help of bootloader, the bootloader code might interact with >> registers that the Linux code writes with zero. >> >> Keeping it for registers where driver writes zero should also help if the >> serial IPs power will be off during suspend, thus registers restored to non >> zero default values (by HW) after resume. > > Sure, the driver would have to write zero to the register anyway. > > While storing the suspend_cacheable flag wouldn't cost any storage > space anymore using bitfields, I am wondering if it would be worthwhile > to have explicit code to save/restore registers, instead of looping > over all of them and checking the flag. I.e. > > u16 saved_scmsr; > u16 saved_scscr; > u8 saved_scbrr; > ... > u8 saved_semr; > > /* Save omnipresent registers */ > s->saved_scmsr = sci_serial_in(port, SCSMR); > ... > /* Save optional registers */ > if (sci_getreg(port, SEMR)->size) > s->saved_semr = sci_serial_in(port, SEMR); > > That would make it apply to all SCI variants, not just for SCIFA, > while increasing sci_port by only 10 bytes/port. And 10 bytes/port is > probably not worth to be protected by an #ifdef... That was the other approach I thought about when working on this patch. I chose the one proposed in this patch as it looked to me simpler to extend to other registers, if needed (just enable proper flag in sci_port_params[]). And needed less changes for the code saving/restoring the registers. If you prefer your version let me know and I'll switch to it. Thank you, Claudiu > > Thoughts? > > Gr{oetje,eeting}s, > > Geert >
Hi Claudiu, On Mon, 27 Jan 2025 at 13:23, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > On 27.01.2025 11:19, Geert Uytterhoeven wrote: > > On Mon, 27 Jan 2025 at 09:44, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > >> On 24.01.2025 12:53, Geert Uytterhoeven wrote: > >>> On Mon, Jan 20, 2025 at 2:09 PM Claudiu <claudiu.beznea@tuxon.dev> 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 > >>> > >>> cached > >>> > >>>> 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> > > While storing the suspend_cacheable flag wouldn't cost any storage > > space anymore using bitfields, I am wondering if it would be worthwhile > > to have explicit code to save/restore registers, instead of looping > > over all of them and checking the flag. I.e. > > > > u16 saved_scmsr; > > u16 saved_scscr; > > u8 saved_scbrr; > > ... > > u8 saved_semr; > > > > /* Save omnipresent registers */ > > s->saved_scmsr = sci_serial_in(port, SCSMR); > > ... > > /* Save optional registers */ > > if (sci_getreg(port, SEMR)->size) > > s->saved_semr = sci_serial_in(port, SEMR); > > > > That would make it apply to all SCI variants, not just for SCIFA, > > while increasing sci_port by only 10 bytes/port. And 10 bytes/port is > > probably not worth to be protected by an #ifdef... > > That was the other approach I thought about when working on this patch. I > chose the one proposed in this patch as it looked to me simpler to extend > to other registers, if needed (just enable proper flag in > sci_port_params[]). And needed less changes for the code saving/restoring > the registers. > > If you prefer your version let me know and I'll switch to it. As this driver is also used on smaller systems (e.g. SH), I'd go for the solution that leads to the smallest increase of code size and memory consumption. Gr{oetje,eeting}s, Geert
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index b1ea48f38248..ae43237dd684 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; @@ -300,17 +303,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, @@ -3374,6 +3377,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 +3550,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; } @@ -3560,8 +3585,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; }