[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Juergen Borleis]

Whenever the UART device driver gets closed from userland, the driver
disables the UART unit and then stops its clock to save power.

The bit which disabled the UART unit is described as:

  "UART Enable. If this bit is set to 1, the UART is enabled. Data
  transmission and reception occurs for the UART signals. When the
  UART is disabled in the middle of transmission or reception, it
  completes the current character before stopping."

The important part is the "it completes the current character". Whenever
a reception is ongoing when the UART gets disabled (including the clock
off) the statemachine freezes and "remembers" this state on the next
open() and re-enabling of the unit's clock.

In this case we end up receiving an additional bogus character
immediately.

The solution in this change is to move the AUART unit into its reset
state on close() and only release it from its reset state on the next
open().

Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
---

Notes:
    v3:
     - missing newline added to an error message
    
    v2:
     - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better reflect
       what it really does
     - adapt the delay in mxs_auart_keep_reset() to wait for the reset of the
       AURAT unit to what is used in mxs_auart_reset()
     - typo fixed

 drivers/tty/serial/mxs-auart.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 13cf773..135ee6c 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -858,6 +858,30 @@ static void mxs_auart_reset(struct uart_port *u)
 	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
 }
 
+static void mxs_auart_keep_reset(struct uart_port *u)
+{
+	int i;
+	u32 reg;
+
+	reg = readl(u->membase + AUART_CTRL0);
+	/* if already in reset state, keep it untouched */
+	if (reg & AUART_CTRL0_SFTRST)
+		return;
+
+	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
+	writel(AUART_CTRL0_SFTRST, u->membase + AUART_CTRL0_SET);
+
+	for (i = 0; i < 10000; i++) {
+		reg = readl(u->membase + AUART_CTRL0);
+		/* reset is finished when the clock is gated */
+		if (reg & AUART_CTRL0_CLKGATE)
+			return;
+		udelay(3);
+	}
+
+	dev_err(u->dev, "Failed to reset the unit.\n");
+}
+
 static int mxs_auart_startup(struct uart_port *u)
 {
 	int ret;
@@ -867,7 +891,10 @@ static int mxs_auart_startup(struct uart_port *u)
 	if (ret)
 		return ret;
 
-	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
+	/* reset the unit if not aleady done */
+	mxs_auart_keep_reset(u);
+	/* bring it out of reset now */
+	mxs_auart_reset(u);
 
 	writel(AUART_CTRL2_UARTEN, u->membase + AUART_CTRL2_SET);
 
@@ -899,13 +926,8 @@ static void mxs_auart_shutdown(struct uart_port *u)
 	if (auart_dma_enabled(s))
 		mxs_auart_dma_exit(s);
 
-	writel(AUART_CTRL2_UARTEN, u->membase + AUART_CTRL2_CLR);
-
-	writel(AUART_INTR_RXIEN | AUART_INTR_RTIEN | AUART_INTR_CTSMIEN,
-			u->membase + AUART_INTR_CLR);
-
-	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_SET);
-
+	/* reset the unit and keep it in reset state */
+	mxs_auart_keep_reset(u);
 	clk_disable_unprepare(s->clk);
 }
 
-- 
2.1.4

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Stefan Wahren]

> Whenever the UART device driver gets closed from userland, the driver
> disables the UART unit and then stops its clock to save power.
>
> The bit which disabled the UART unit is described as:
>
>   "UART Enable. If this bit is set to 1, the UART is enabled. Data
>   transmission and reception occurs for the UART signals. When the
>   UART is disabled in the middle of transmission or reception, it
>   completes the current character before stopping."
>
> The important part is the "it completes the current character". Whenever
> a reception is ongoing when the UART gets disabled (including the clock
> off) the statemachine freezes and "remembers" this state on the next
> open() and re-enabling of the unit's clock.
>
> In this case we end up receiving an additional bogus character
> immediately.
>
> The solution in this change is to move the AUART unit into its reset
> state on close() and only release it from its reset state on the next
> open().
>
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> ---

Acked-by: Stefan Wahren <stefan.wahren@i2se.com>

Thanks
Stefan

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Peter Hurley]

Hi Juergen,

On 07/16/2015 03:40 AM, Juergen Borleis wrote:
> Whenever the UART device driver gets closed from userland, the driver
> disables the UART unit and then stops its clock to save power.
> 
> The bit which disabled the UART unit is described as:
> 
>   "UART Enable. If this bit is set to 1, the UART is enabled. Data
>   transmission and reception occurs for the UART signals. When the
>   UART is disabled in the middle of transmission or reception, it
>   completes the current character before stopping."
> 
> The important part is the "it completes the current character". Whenever
> a reception is ongoing when the UART gets disabled (including the clock
> off) the statemachine freezes and "remembers" this state on the next
> open() and re-enabling of the unit's clock.
> 
> In this case we end up receiving an additional bogus character
> immediately.
> 
> The solution in this change is to move the AUART unit into its reset
> state on close() and only release it from its reset state on the next
> open().
> 
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> ---
> 
> Notes:
>     v3:
>      - missing newline added to an error message
>     
>     v2:
>      - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better reflect
>        what it really does
>      - adapt the delay in mxs_auart_keep_reset() to wait for the reset of the
>        AURAT unit to what is used in mxs_auart_reset()

The function names and semantics are not clear. See below.

>      - typo fixed
> 
>  drivers/tty/serial/mxs-auart.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 13cf773..135ee6c 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -858,6 +858,30 @@ static void mxs_auart_reset(struct uart_port *u)
>  	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>  }
>  
> +static void mxs_auart_keep_reset(struct uart_port *u)
> +{
> +	int i;
> +	u32 reg;
> +
> +	reg = readl(u->membase + AUART_CTRL0);
> +	/* if already in reset state, keep it untouched */
> +	if (reg & AUART_CTRL0_SFTRST)
> +		return;
> +
> +	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
> +	writel(AUART_CTRL0_SFTRST, u->membase + AUART_CTRL0_SET);
> +
> +	for (i = 0; i < 10000; i++) {
> +		reg = readl(u->membase + AUART_CTRL0);
> +		/* reset is finished when the clock is gated */
> +		if (reg & AUART_CTRL0_CLKGATE)
> +			return;
> +		udelay(3);
> +	}
> +
> +	dev_err(u->dev, "Failed to reset the unit.\n");
> +}
> +
>  static int mxs_auart_startup(struct uart_port *u)
>  {
>  	int ret;
> @@ -867,7 +891,10 @@ static int mxs_auart_startup(struct uart_port *u)
>  	if (ret)
>  		return ret;
>  
> -	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
> +	/* reset the unit if not aleady done */
> +	mxs_auart_keep_reset(u);

Why is this performing a soft-reset on open now? Didn't mxs_auart_shutdown()
already leave it in this state?

Because if you're performing a soft-reset deliberately as part of the
initial condition, you make no mention of that in the changelog.


> +	/* bring it out of reset now */
> +	mxs_auart_reset(u);

mxs_auart_reset() is really not resetting but simply performing a block enable.
Isn't there a generic block enable for the iMX.2x SoCs? (Maybe there should be)

The names of these functions don't match expected operations of startup().
Start up should be enabling the device, not keeping it in reset.

And "keep reset" immediately followed by "reset" makes no sense.

Regards,
Peter Hurley


>  	writel(AUART_CTRL2_UARTEN, u->membase + AUART_CTRL2_SET);
>  
> @@ -899,13 +926,8 @@ static void mxs_auart_shutdown(struct uart_port *u)
>  	if (auart_dma_enabled(s))
>  		mxs_auart_dma_exit(s);
>  
> -	writel(AUART_CTRL2_UARTEN, u->membase + AUART_CTRL2_CLR);
> -
> -	writel(AUART_INTR_RXIEN | AUART_INTR_RTIEN | AUART_INTR_CTSMIEN,
> -			u->membase + AUART_INTR_CLR);
> -
> -	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_SET);
> -
> +	/* reset the unit and keep it in reset state */
> +	mxs_auart_keep_reset(u);
>  	clk_disable_unprepare(s->clk);
>  }
>  
> 

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Philipp Zabel]

Am Freitag, den 17.07.2015, 09:10 -0400 schrieb Peter Hurley:
> Hi Juergen,
> 
> On 07/16/2015 03:40 AM, Juergen Borleis wrote:
> > Whenever the UART device driver gets closed from userland, the driver
> > disables the UART unit and then stops its clock to save power.
> > 
> > The bit which disabled the UART unit is described as:
> > 
> >   "UART Enable. If this bit is set to 1, the UART is enabled. Data
> >   transmission and reception occurs for the UART signals. When the
> >   UART is disabled in the middle of transmission or reception, it
> >   completes the current character before stopping."
> > 
> > The important part is the "it completes the current character". Whenever
> > a reception is ongoing when the UART gets disabled (including the clock
> > off) the statemachine freezes and "remembers" this state on the next
> > open() and re-enabling of the unit's clock.
> > 
> > In this case we end up receiving an additional bogus character
> > immediately.
> > 
> > The solution in this change is to move the AUART unit into its reset
> > state on close() and only release it from its reset state on the next
> > open().
> > 
> > Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> > ---
> > 
> > Notes:
> >     v3:
> >      - missing newline added to an error message
> >     
> >     v2:
> >      - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better reflect
> >        what it really does
> >      - adapt the delay in mxs_auart_keep_reset() to wait for the reset of the
> >        AURAT unit to what is used in mxs_auart_reset()
> 
> The function names and semantics are not clear. See below.
[...]
> > +	/* bring it out of reset now */
> > +	mxs_auart_reset(u);
> 
> mxs_auart_reset() is really not resetting but simply performing a block enable.
> Isn't there a generic block enable for the iMX.2x SoCs? (Maybe there should be)
> 
> The names of these functions don't match expected operations of startup().
> Start up should be enabling the device, not keeping it in reset.
> 
> And "keep reset" immediately followed by "reset" makes no sense.

I'd call the pair mxs_auart_reset_assert and mxs_auart_reset_deassert.

regards
Philipp

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Juergen Borleis]

Hi Peter,

On Friday 17 July 2015 15:10:06 Peter Hurley wrote:
> On 07/16/2015 03:40 AM, Juergen Borleis wrote:
> > Whenever the UART device driver gets closed from userland, the driver
> > disables the UART unit and then stops its clock to save power.
> >
> > The bit which disabled the UART unit is described as:
> >
> >   "UART Enable. If this bit is set to 1, the UART is enabled. Data
> >   transmission and reception occurs for the UART signals. When the
> >   UART is disabled in the middle of transmission or reception, it
> >   completes the current character before stopping."
> >
> > The important part is the "it completes the current character". Whenever
> > a reception is ongoing when the UART gets disabled (including the clock
> > off) the statemachine freezes and "remembers" this state on the next
> > open() and re-enabling of the unit's clock.
> >
> > In this case we end up receiving an additional bogus character
> > immediately.
> >
> > The solution in this change is to move the AUART unit into its reset
> > state on close() and only release it from its reset state on the next
> > open().
> >
> > Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> > ---
> >
> > Notes:
> >     v3:
> >      - missing newline added to an error message
> >
> >     v2:
> >      - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better
> > reflect what it really does
> >      - adapt the delay in mxs_auart_keep_reset() to wait for the reset of
> > the AURAT unit to what is used in mxs_auart_reset()
>
> The function names and semantics are not clear. See below.
>
> >      - typo fixed
> >
> >  drivers/tty/serial/mxs-auart.c | 38
> > ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+),
> > 8 deletions(-)
> >
> > diff --git a/drivers/tty/serial/mxs-auart.c
> > b/drivers/tty/serial/mxs-auart.c index 13cf773..135ee6c 100644
> > --- a/drivers/tty/serial/mxs-auart.c
> > +++ b/drivers/tty/serial/mxs-auart.c
> > @@ -858,6 +858,30 @@ static void mxs_auart_reset(struct uart_port *u)
> >  	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
> >  }
> >
> > +static void mxs_auart_keep_reset(struct uart_port *u)
> > +{
> > +	int i;
> > +	u32 reg;
> > +
> > +	reg = readl(u->membase + AUART_CTRL0);
> > +	/* if already in reset state, keep it untouched */
> > +	if (reg & AUART_CTRL0_SFTRST)
> > +		return;
> > +
> > +	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
> > +	writel(AUART_CTRL0_SFTRST, u->membase + AUART_CTRL0_SET);
> > +
> > +	for (i = 0; i < 10000; i++) {
> > +		reg = readl(u->membase + AUART_CTRL0);
> > +		/* reset is finished when the clock is gated */
> > +		if (reg & AUART_CTRL0_CLKGATE)
> > +			return;
> > +		udelay(3);
> > +	}
> > +
> > +	dev_err(u->dev, "Failed to reset the unit.\n");
> > +}
> > +
> >  static int mxs_auart_startup(struct uart_port *u)
> >  {
> >  	int ret;
> > @@ -867,7 +891,10 @@ static int mxs_auart_startup(struct uart_port *u)
> >  	if (ret)
> >  		return ret;
> >
> > -	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
> > +	/* reset the unit if not aleady done */
> > +	mxs_auart_keep_reset(u);
>
> Why is this performing a soft-reset on open now? Didn't
> mxs_auart_shutdown() already leave it in this state?

The probe function let the device left without reset. That is why I do a reset 
here (which is skipped if the device is already in reset state).

> Because if you're performing a soft-reset deliberately as part of the
> initial condition, you make no mention of that in the changelog.

If I switch the device into the reset state immediately after the probe 
function has finished, I could skip this additional reset.

> > +	/* bring it out of reset now */
> > +	mxs_auart_reset(u);
>
> mxs_auart_reset() is really not resetting but simply performing a block
> enable. Isn't there a generic block enable for the iMX.2x SoCs? (Maybe
> there should be)
>
> The names of these functions don't match expected operations of startup().
> Start up should be enabling the device, not keeping it in reset.
>
> And "keep reset" immediately followed by "reset" makes no sense.

mxs_auart_reset() is a bad name from the beginning. But I just wanted to keep 
this change small and only change things that are really required.
I will try with a V4.

Regards,
Juergen

-- 
Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| Juergen Borleis ? ? ? ? ? ? |
Linux Solutions for Science and Industry ? ? ?| Phone: +49-5121-206917-5128 |
Peiner Str. 6-8, 31137 Hildesheim, Germany ? ?| Fax: ? +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 ? ? ? ? ? ? ?| http://www.pengutronix.de/ ?|

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Peter Hurley]

On 07/20/2015 04:04 AM, Juergen Borleis wrote:
> Hi Peter,
> 
> On Friday 17 July 2015 15:10:06 Peter Hurley wrote:
>> On 07/16/2015 03:40 AM, Juergen Borleis wrote:
>>> Whenever the UART device driver gets closed from userland, the driver
>>> disables the UART unit and then stops its clock to save power.
>>>
>>> The bit which disabled the UART unit is described as:
>>>
>>>   "UART Enable. If this bit is set to 1, the UART is enabled. Data
>>>   transmission and reception occurs for the UART signals. When the
>>>   UART is disabled in the middle of transmission or reception, it
>>>   completes the current character before stopping."
>>>
>>> The important part is the "it completes the current character". Whenever
>>> a reception is ongoing when the UART gets disabled (including the clock
>>> off) the statemachine freezes and "remembers" this state on the next
>>> open() and re-enabling of the unit's clock.
>>>
>>> In this case we end up receiving an additional bogus character
>>> immediately.
>>>
>>> The solution in this change is to move the AUART unit into its reset
>>> state on close() and only release it from its reset state on the next
>>> open().
>>>
>>> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
>>> ---
>>>
>>> Notes:
>>>     v3:
>>>      - missing newline added to an error message
>>>
>>>     v2:
>>>      - rename mxs_auart_do_reset() to mxs_auart_keep_reset() to better
>>> reflect what it really does
>>>      - adapt the delay in mxs_auart_keep_reset() to wait for the reset of
>>> the AURAT unit to what is used in mxs_auart_reset()
>>
>> The function names and semantics are not clear. See below.
>>
>>>      - typo fixed
>>>
>>>  drivers/tty/serial/mxs-auart.c | 38
>>> ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+),
>>> 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/mxs-auart.c
>>> b/drivers/tty/serial/mxs-auart.c index 13cf773..135ee6c 100644
>>> --- a/drivers/tty/serial/mxs-auart.c
>>> +++ b/drivers/tty/serial/mxs-auart.c
>>> @@ -858,6 +858,30 @@ static void mxs_auart_reset(struct uart_port *u)
>>>  	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>>  }
>>>
>>> +static void mxs_auart_keep_reset(struct uart_port *u)
>>> +{
>>> +	int i;
>>> +	u32 reg;
>>> +
>>> +	reg = readl(u->membase + AUART_CTRL0);
>>> +	/* if already in reset state, keep it untouched */
>>> +	if (reg & AUART_CTRL0_SFTRST)
>>> +		return;
>>> +
>>> +	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>> +	writel(AUART_CTRL0_SFTRST, u->membase + AUART_CTRL0_SET);
>>> +
>>> +	for (i = 0; i < 10000; i++) {
>>> +		reg = readl(u->membase + AUART_CTRL0);
>>> +		/* reset is finished when the clock is gated */
>>> +		if (reg & AUART_CTRL0_CLKGATE)
>>> +			return;
>>> +		udelay(3);
>>> +	}
>>> +
>>> +	dev_err(u->dev, "Failed to reset the unit.\n");
>>> +}
>>> +
>>>  static int mxs_auart_startup(struct uart_port *u)
>>>  {
>>>  	int ret;
>>> @@ -867,7 +891,10 @@ static int mxs_auart_startup(struct uart_port *u)
>>>  	if (ret)
>>>  		return ret;
>>>
>>> -	writel(AUART_CTRL0_CLKGATE, u->membase + AUART_CTRL0_CLR);
>>> +	/* reset the unit if not aleady done */
>>> +	mxs_auart_keep_reset(u);
>>
>> Why is this performing a soft-reset on open now? Didn't
>> mxs_auart_shutdown() already leave it in this state?
> 
> The probe function let the device left without reset. That is why I do a reset 
> here (which is skipped if the device is already in reset state).

So you're "fixing" something else (reset @ probe) without _any_
indication in the changelog?


>> Because if you're performing a soft-reset deliberately as part of the
>> initial condition, you make no mention of that in the changelog.
> 
> If I switch the device into the reset state immediately after the probe 
> function has finished, I could skip this additional reset.
> 
>>> +	/* bring it out of reset now */
>>> +	mxs_auart_reset(u);
>>
>> mxs_auart_reset() is really not resetting but simply performing a block
>> enable. Isn't there a generic block enable for the iMX.2x SoCs? (Maybe
>> there should be)
>>
>> The names of these functions don't match expected operations of startup().
>> Start up should be enabling the device, not keeping it in reset.
>>
>> And "keep reset" immediately followed by "reset" makes no sense.
> 
> mxs_auart_reset() is a bad name from the beginning. But I just wanted to keep 
> this change small and only change things that are really required.
> I will try with a V4.

ISTM this should be split into 2 or 3 separate patches:
1. Rename mxs_auart_reset()
2. Fix stale rx on re-open
3. Reset @ probe


Regards,
Peter Hurley

[PATCH v3] serial: mxs-auart: keep the AUART unit in reset state when not in use

[Juergen Borleis]

Hi Peter,

On Monday 20 July 2015 15:00:21 Peter Hurley wrote:
> [...]
> > mxs_auart_reset() is a bad name from the beginning. But I just wanted to
> > keep this change small and only change things that are really required. I
> > will try with a V4.
>
> ISTM this should be split into 2 or 3 separate patches:
> 1. Rename mxs_auart_reset()
> 2. Fix stale rx on re-open
> 3. Reset @ probe

Okay.

Regards,
Juergen

-- 
Pengutronix e.K. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| Juergen Borleis ? ? ? ? ? ? |
Industrial Linux Solutions ? ? ? ? ? ? ? ? ? ?| http://www.pengutronix.de/ ?|