LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] serial: 8250: omap: Fix idling of clocks for unused uarts
@ 2018-05-02 17:15 Tony Lindgren
  2018-05-03 10:18 ` Vignesh R
  0 siblings, 1 reply; 3+ messages in thread
From: Tony Lindgren @ 2018-05-02 17:15 UTC (permalink / raw)
  To: Peter Hurley, Greg Kroah-Hartman
  Cc: Peter Ujfalusi, Sebastian Andrzej Siewior, Vignesh R,
	linux-serial, linux-omap, linux-kernel, Keerthy,
	Matthijs van Duin, Sekhar Nori, Tero Kristo

I noticed that unused UARTs won't necessarily idle properly always
unless at least one byte tx transfer is done first.

After some debugging I narrowed down the problem to the scr register
dma configuration bits that need to be set before softreset for the
clocks to idle. Unless we do this, the module clkctrl idlest bits
may be set to 1 instead of 3 meaning the clock will never idle and
is blocking deeper idle states for the whole domain.

This might be related to the configuration done by the bootloader
or kexec booting where certain configurations cause the 8250 or
the clkctrl clock to jam in a way where setting of the scr bits
and reset is needed to clear it. I've tried diffing the 8250
registers for the various modes, but did not see anything specific.
So far I've only seen this on omap4 but I'm suspecting this might
also happen on the other clkctrl using SoCs considering they
already have a quirk enabled for UART_ERRATA_CLOCK_DISABLE.

Let's fix the issue by configuring scr before reset for basic dma
even if we don't use it. The scr register will be reset when we do
softreset few lines after, and we restore scr on resume. We should
do this for all the SoCs with UART_ERRATA_CLOCK_DISABLE quirk flag
set since the ones with UART_ERRATA_CLOCK_DISABLE are all based
using clkctrl similar to omap4.

Looks like both OMAP_UART_SCR_DMAMODE_1 | OMAP_UART_SCR_DMAMODE_CTL
bits are needed for the clkctrl to idle after a softreset.

And we need to add omap4 to also use the UART_ERRATA_CLOCK_DISABLE
for the related workaround to be enabled. This same compatible
value will also be used for omap5.

Fixes: cdb929e4452a ("serial: 8250_omap: workaround errata around
idling UART after using DMA")
Cc: Keerthy <j-keerthy@ti.com>
Cc: Matthijs van Duin <matthijsvanduin@gmail.com>
Cc: Sekhar Nori <nsekhar@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/tty/serial/8250/8250_omap.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1110,13 +1110,14 @@ static int omap8250_no_handle_irq(struct uart_port *port)
 	return 0;
 }
 
+static const u8 omap4_habit = UART_ERRATA_CLOCK_DISABLE;
 static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE;
 static const u8 dra742_habit = UART_ERRATA_CLOCK_DISABLE;
 
 static const struct of_device_id omap8250_dt_ids[] = {
 	{ .compatible = "ti,omap2-uart" },
 	{ .compatible = "ti,omap3-uart" },
-	{ .compatible = "ti,omap4-uart" },
+	{ .compatible = "ti,omap4-uart", .data = &omap4_habit, },
 	{ .compatible = "ti,am3352-uart", .data = &am3352_habit, },
 	{ .compatible = "ti,am4372-uart", .data = &am3352_habit, },
 	{ .compatible = "ti,dra742-uart", .data = &dra742_habit, },
@@ -1362,6 +1363,16 @@ static int omap8250_soft_reset(struct device *dev)
 	int sysc;
 	int syss;
 
+	/*
+	 * At least on omap4, unused uarts may not idle after reset without
+	 * a basic scr dma configuration even with no dma in use. The
+	 * module clkctrl status bit will stay set blocking idle for the
+	 * whole clockdomain. The softreset below will clear scr, and we
+	 * restore it on resume so this is safe to do on all SoCs needing
+	 * omap8250_soft_reset() quirk.
+	 */
+	serial_out(up, UART_OMAP_SCR,
+		   OMAP_UART_SCR_DMAMODE_1 | OMAP_UART_SCR_DMAMODE_CTL);
 	sysc = serial_in(up, UART_OMAP_SYSC);
 
 	/* softreset the UART */
-- 
2.17.0

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

* Re: [PATCH] serial: 8250: omap: Fix idling of clocks for unused uarts
  2018-05-02 17:15 [PATCH] serial: 8250: omap: Fix idling of clocks for unused uarts Tony Lindgren
@ 2018-05-03 10:18 ` Vignesh R
  2018-05-03 14:31   ` Tony Lindgren
  0 siblings, 1 reply; 3+ messages in thread
From: Vignesh R @ 2018-05-03 10:18 UTC (permalink / raw)
  To: Tony Lindgren, Peter Hurley, Greg Kroah-Hartman
  Cc: Peter Ujfalusi, Sebastian Andrzej Siewior, linux-serial,
	linux-omap, linux-kernel, Keerthy, Matthijs van Duin,
	Sekhar Nori, Tero Kristo



On Wednesday 02 May 2018 10:45 PM, Tony Lindgren wrote:
> I noticed that unused UARTs won't necessarily idle properly always
> unless at least one byte tx transfer is done first.
> 
> After some debugging I narrowed down the problem to the scr register
> dma configuration bits that need to be set before softreset for the
> clocks to idle. Unless we do this, the module clkctrl idlest bits
> may be set to 1 instead of 3 meaning the clock will never idle and
> is blocking deeper idle states for the whole domain.
> 
> This might be related to the configuration done by the bootloader
> or kexec booting where certain configurations cause the 8250 or
> the clkctrl clock to jam in a way where setting of the scr bits
> and reset is needed to clear it. I've tried diffing the 8250
> registers for the various modes, but did not see anything specific.
> So far I've only seen this on omap4 but I'm suspecting this might
> also happen on the other clkctrl using SoCs considering they
> already have a quirk enabled for UART_ERRATA_CLOCK_DISABLE.
> 

That's interesting! We do have AM437x suspend/resume working without
this workaround (UARTs on AM437x does not use DMA) and UART IPs clkctrl
do go to idle state. Seems like a OMAP4 specific issue.

> Let's fix the issue by configuring scr before reset for basic dma
> even if we don't use it. The scr register will be reset when we do
> softreset few lines after, and we restore scr on resume. We should
> do this for all the SoCs with UART_ERRATA_CLOCK_DISABLE quirk flag
> set since the ones with UART_ERRATA_CLOCK_DISABLE are all based
> using clkctrl similar to omap4.
> 
> Looks like both OMAP_UART_SCR_DMAMODE_1 | OMAP_UART_SCR_DMAMODE_CTL
> bits are needed for the clkctrl to idle after a softreset.
> 
> And we need to add omap4 to also use the UART_ERRATA_CLOCK_DISABLE
> for the related workaround to be enabled. This same compatible
> value will also be used for omap5.
> 
> Fixes: cdb929e4452a ("serial: 8250_omap: workaround errata around
> idling UART after using DMA")
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Matthijs van Duin <matthijsvanduin@gmail.com>
> Cc: Sekhar Nori <nsekhar@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1110,13 +1110,14 @@ static int omap8250_no_handle_irq(struct uart_port *port)
>  	return 0;
>  }
>  
> +static const u8 omap4_habit = UART_ERRATA_CLOCK_DISABLE;
>  static const u8 am3352_habit = OMAP_DMA_TX_KICK | UART_ERRATA_CLOCK_DISABLE;
>  static const u8 dra742_habit = UART_ERRATA_CLOCK_DISABLE;
>  
>  static const struct of_device_id omap8250_dt_ids[] = {
>  	{ .compatible = "ti,omap2-uart" },
>  	{ .compatible = "ti,omap3-uart" },
> -	{ .compatible = "ti,omap4-uart" },
> +	{ .compatible = "ti,omap4-uart", .data = &omap4_habit, },
>  	{ .compatible = "ti,am3352-uart", .data = &am3352_habit, },
>  	{ .compatible = "ti,am4372-uart", .data = &am3352_habit, },
>  	{ .compatible = "ti,dra742-uart", .data = &dra742_habit, },
> @@ -1362,6 +1363,16 @@ static int omap8250_soft_reset(struct device *dev)
>  	int sysc;
>  	int syss;
>  
> +	/*
> +	 * At least on omap4, unused uarts may not idle after reset without
> +	 * a basic scr dma configuration even with no dma in use. The
> +	 * module clkctrl status bit will stay set blocking idle for the
> +	 * whole clockdomain. The softreset below will clear scr, and we
> +	 * restore it on resume so this is safe to do on all SoCs needing
> +	 * omap8250_soft_reset() quirk.
> +	 */
> +	serial_out(up, UART_OMAP_SCR,
> +		   OMAP_UART_SCR_DMAMODE_1 | OMAP_UART_SCR_DMAMODE_CTL);

Comment in omap8250_update_scr() warns not to set these two bits in a
single register write because this may lead to malfunction. I would
recommend to split this into two writes.

>  	sysc = serial_in(up, UART_OMAP_SYSC);
>  
>  	/* softreset the UART */
> 

-- 
Regards
Vignesh

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

* Re: [PATCH] serial: 8250: omap: Fix idling of clocks for unused uarts
  2018-05-03 10:18 ` Vignesh R
@ 2018-05-03 14:31   ` Tony Lindgren
  0 siblings, 0 replies; 3+ messages in thread
From: Tony Lindgren @ 2018-05-03 14:31 UTC (permalink / raw)
  To: Vignesh R
  Cc: Peter Hurley, Greg Kroah-Hartman, Peter Ujfalusi,
	Sebastian Andrzej Siewior, linux-serial, linux-omap,
	linux-kernel, Keerthy, Matthijs van Duin, Sekhar Nori,
	Tero Kristo

* Vignesh R <vigneshr@ti.com> [180503 10:20]:
> 
> 
> On Wednesday 02 May 2018 10:45 PM, Tony Lindgren wrote:
> > I noticed that unused UARTs won't necessarily idle properly always
> > unless at least one byte tx transfer is done first.
> > 
> > After some debugging I narrowed down the problem to the scr register
> > dma configuration bits that need to be set before softreset for the
> > clocks to idle. Unless we do this, the module clkctrl idlest bits
> > may be set to 1 instead of 3 meaning the clock will never idle and
> > is blocking deeper idle states for the whole domain.
> > 
> > This might be related to the configuration done by the bootloader
> > or kexec booting where certain configurations cause the 8250 or
> > the clkctrl clock to jam in a way where setting of the scr bits
> > and reset is needed to clear it. I've tried diffing the 8250
> > registers for the various modes, but did not see anything specific.
> > So far I've only seen this on omap4 but I'm suspecting this might
> > also happen on the other clkctrl using SoCs considering they
> > already have a quirk enabled for UART_ERRATA_CLOCK_DISABLE.
> > 
> 
> That's interesting! We do have AM437x suspend/resume working without
> this workaround (UARTs on AM437x does not use DMA) and UART IPs clkctrl
> do go to idle state. Seems like a OMAP4 specific issue.

Yeah seems to be omap4 specific. I did not see this yesterday
on my am437x-idk or beagle-x15 after clearing status = "disabled"
for all ports and idling them.

> Comment in omap8250_update_scr() warns not to set these two bits in a
> single register write because this may lead to malfunction. I would
> recommend to split this into two writes.

OK I'll split the write into two parts.

Regards,

Tony

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

end of thread, other threads:[~2018-05-03 14:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-02 17:15 [PATCH] serial: 8250: omap: Fix idling of clocks for unused uarts Tony Lindgren
2018-05-03 10:18 ` Vignesh R
2018-05-03 14:31   ` Tony Lindgren

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).