| Message ID | 1436884864-6782-1-git-send-email-jbe@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
Hello Juergen, > Juergen Borleis <jbe@pengutronix.de> hat am 14. Juli 2015 um 16:41 > geschrieben: > > > [...] > --- > 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..f42b6ad 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_do_reset(struct uart_port *u) i think the naming of this new function is too similiar to mxs_auart_reset() and doesn't represent the exact behavior. How about mxs_auart_keep_reset() or mxs_auart_gate()? > +{ > + 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 < 1000; i++) { > + reg = readl(u->membase + AUART_CTRL0); > + /* reset is finished when the clock is gated */ > + if (reg & AUART_CTRL0_CLKGATE) > + return; > + udelay(10); The delay in mxs_auart_reset() has a value of 3 microseconds. Why not the same here? > + } > + > + dev_err(u->dev, "Failed to reset the unit."); > +} > + > 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 aready done */ Just a typo: already? Thanks Stefan
Hi Stefan, On Tuesday 14 July 2015 19:33:04 Stefan Wahren wrote: > [...] > > diff --git a/drivers/tty/serial/mxs-auart.c > > b/drivers/tty/serial/mxs-auart.c index 13cf773..f42b6ad 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_do_reset(struct uart_port *u) > > i think the naming of this new function is too similiar to > mxs_auart_reset() and doesn't represent the exact behavior. How about > mxs_auart_keep_reset() or mxs_auart_gate()? Hmm, maybe mxs_auart_reset() should be renamed instead, because it terminates the reset to this unit and make it work (called when the driver's probe function is running). But okay, mxs_auart_keep_reset() names more what it really does. > [...] > > + udelay(10); > > The delay in mxs_auart_reset() has a value of 3 microseconds. Why not the > same here? Hmm, good question. And why 3 us? :) I'm going to change it to 3 us as well. > [...] > > + /* reset the unit if not aready done */ > > Just a typo: already? Ups. Yes. Will fix. Thanks! Juergen
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c index 13cf773..f42b6ad 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_do_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 < 1000; i++) { + reg = readl(u->membase + AUART_CTRL0); + /* reset is finished when the clock is gated */ + if (reg & AUART_CTRL0_CLKGATE) + return; + udelay(10); + } + + dev_err(u->dev, "Failed to reset the unit."); +} + 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 aready done */ + mxs_auart_do_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_do_reset(u); clk_disable_unprepare(s->clk); }
Whenever the UART device driver gets closed from userland, the driver disables the UART unit and then stops the clocks 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> --- drivers/tty/serial/mxs-auart.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-)