Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock
@ 2019-08-20 12:18 Magnus Damm
  2019-08-29  9:21 ` Geert Uytterhoeven
  2020-02-07 11:54 ` Geert Uytterhoeven
  0 siblings, 2 replies; 3+ messages in thread
From: Magnus Damm @ 2019-08-20 12:18 UTC (permalink / raw)
  To: linux-renesas-soc; +Cc: Magnus Damm, geert

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";
 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock
  2019-08-20 12:18 [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock Magnus Damm
@ 2019-08-29  9:21 ` Geert Uytterhoeven
  2020-02-07 11:54 ` Geert Uytterhoeven
  1 sibling, 0 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2019-08-29  9:21 UTC (permalink / raw)
  To: Magnus Damm; +Cc: Linux-Renesas

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock
  2019-08-20 12:18 [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock Magnus Damm
  2019-08-29  9:21 ` Geert Uytterhoeven
@ 2020-02-07 11:54 ` Geert Uytterhoeven
  1 sibling, 0 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2020-02-07 11:54 UTC (permalink / raw)
  To: Magnus Damm; +Cc: Linux-Renesas

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-20 12:18 [PATCH/RFC] serial: sh-sci: Update uartclk based on selected clock Magnus Damm
2019-08-29  9:21 ` Geert Uytterhoeven
2020-02-07 11:54 ` Geert Uytterhoeven

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git