Re: [PATCH v4 1/4] serial: sh-sci: Update the suspend/resume support

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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
>