[PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock

[PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

I noticed that uartclk never gets updated as it is today.

This compile tested patch makes sure uartclk is in sync with
whatever clock is selected during sci_set_termios(). Exposing
the actual value to user space seems like a good plan to me.

Another semi-related issue:

The ->termios() callback in the SCIF driver seems to enable clocks using
runtine pm in sci_port_enable() followed by register accesses and
a before returnng sci_port_enable() disables clocks as well.

The ->pm() callback will enable the port again later on however
on SoCs where the SCIF is in a power domain it looks like the
register contents might get lost due to power down between ->termios()
and ->pm()?

Perhaps the power domain use case is known to be busted?

Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/tty/serial/sh-sci.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--- 0001/drivers/tty/serial/sh-sci.c
+++ work/drivers/tty/serial/sh-sci.c	2019-08-20 20:32:17.680030451 +0900
@@ -129,6 +129,7 @@ struct sci_port {
 	/* Clocks */
 	struct clk		*clks[SCI_NUM_CLKS];
 	unsigned long		clk_rates[SCI_NUM_CLKS];
+	int			sel_clk;
 
 	int			irqs[SCIx_NR_IRQS];
 	char			*irqstr[SCIx_NR_IRQS];
@@ -542,7 +543,11 @@ static void sci_port_enable(struct sci_p
 		clk_prepare_enable(sci_port->clks[i]);
 		sci_port->clk_rates[i] = clk_get_rate(sci_port->clks[i]);
 	}
-	sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
+
+	if (sci_port->sel_clk >= 0)
+		sci_port->port.uartclk = sci_port->clk_rates[sci_port->sel_clk];
+	else
+		sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
 }
 
 static void sci_port_disable(struct sci_port *sci_port)
@@ -2472,6 +2477,7 @@ done:
 		dev_dbg(port->dev, "Using clk %pC for %u%+d bps\n",
 			s->clks[best_clk], baud, min_err);
 
+	s->sel_clk = best_clk;
 	sci_port_enable(s);
 
 	/*
@@ -2785,6 +2791,8 @@ static int sci_init_clocks(struct sci_po
 	struct clk *clk;
 	unsigned int i;
 
+	sci_port->sel_clk = -1; /* invalid */
+
 	if (sci_port->cfg->type == PORT_HSCIF)
 		clk_names[SCI_SCK] = "hsck";
 

Re: [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Aug 20, 2019 at 2:16 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> I noticed that uartclk never gets updated as it is today.
>
> This compile tested patch makes sure uartclk is in sync with
> whatever clock is selected during sci_set_termios(). Exposing
> the actual value to user space seems like a good plan to me.

Thanks for your patch!

> Another semi-related issue:
>
> The ->termios() callback in the SCIF driver seems to enable clocks using
> runtine pm in sci_port_enable() followed by register accesses and
> a before returnng sci_port_enable() disables clocks as well.
>
> The ->pm() callback will enable the port again later on however
> on SoCs where the SCIF is in a power domain it looks like the
> register contents might get lost due to power down between ->termios()
> and ->pm()?
>
> Perhaps the power domain use case is known to be busted?

Have you considered nesting of the various serial core/pm functions?

A quick s2ram test on APE6EVM without no_console_suspend shows that
the A3SP power area is powered down, while the console still works
fine afterwards.

> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
>  drivers/tty/serial/sh-sci.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> --- 0001/drivers/tty/serial/sh-sci.c
> +++ work/drivers/tty/serial/sh-sci.c    2019-08-20 20:32:17.680030451 +0900
> @@ -129,6 +129,7 @@ struct sci_port {
>         /* Clocks */
>         struct clk              *clks[SCI_NUM_CLKS];
>         unsigned long           clk_rates[SCI_NUM_CLKS];
> +       int                     sel_clk;
>
>         int                     irqs[SCIx_NR_IRQS];
>         char                    *irqstr[SCIx_NR_IRQS];
> @@ -542,7 +543,11 @@ static void sci_port_enable(struct sci_p
>                 clk_prepare_enable(sci_port->clks[i]);
>                 sci_port->clk_rates[i] = clk_get_rate(sci_port->clks[i]);
>         }
> -       sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
> +
> +       if (sci_port->sel_clk >= 0)
> +               sci_port->port.uartclk = sci_port->clk_rates[sci_port->sel_clk];
> +       else
> +               sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
>  }
>
>  static void sci_port_disable(struct sci_port *sci_port)
> @@ -2472,6 +2477,7 @@ done:
>                 dev_dbg(port->dev, "Using clk %pC for %u%+d bps\n",
>                         s->clks[best_clk], baud, min_err);
>
> +       s->sel_clk = best_clk;

Might be better to get rid of sci_port.sel_clk, by filling in s->port.uartclk
in sci_set_termios(), right after the call to sci_port_enable() below?

>         sci_port_enable(s);
>
>         /*

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Aug 20, 2019 at 2:16 PM Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> I noticed that uartclk never gets updated as it is today.
>
> This compile tested patch makes sure uartclk is in sync with
> whatever clock is selected during sci_set_termios(). Exposing
> the actual value to user space seems like a good plan to me.

Except that it changes all the time.
What is userspace supposed to do with this value?
Support the deprecated UPF_SPD_CUST flag?
Anything else?

See also "[PATCH] serial: sh-sci: Support custom speed setting"
https://lore.kernel.org/linux-renesas-soc/20200129161955.30562-1-erosca@de.adit-jv.com/

> --- 0001/drivers/tty/serial/sh-sci.c
> +++ work/drivers/tty/serial/sh-sci.c    2019-08-20 20:32:17.680030451 +0900
> @@ -129,6 +129,7 @@ struct sci_port {
>         /* Clocks */
>         struct clk              *clks[SCI_NUM_CLKS];
>         unsigned long           clk_rates[SCI_NUM_CLKS];
> +       int                     sel_clk;
>
>         int                     irqs[SCIx_NR_IRQS];
>         char                    *irqstr[SCIx_NR_IRQS];
> @@ -542,7 +543,11 @@ static void sci_port_enable(struct sci_p
>                 clk_prepare_enable(sci_port->clks[i]);
>                 sci_port->clk_rates[i] = clk_get_rate(sci_port->clks[i]);
>         }
> -       sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
> +
> +       if (sci_port->sel_clk >= 0)
> +               sci_port->port.uartclk = sci_port->clk_rates[sci_port->sel_clk];
> +       else
> +               sci_port->port.uartclk = sci_port->clk_rates[SCI_FCK];
>  }

Iff this is worthwhile, I think it's better to use the highest rate of all
available clock inputs, as that gives a hint about the maximum usable
speed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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