| Message ID | 156630351537.24954.2550542042474735517.sendpatchset@octo (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock | expand |
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
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
--- 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";
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(-)