Hi Magnus,
On Tue, Aug 20, 2019 at 2:16 PM Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
>
> 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@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
