Patchwork [1/2] imx: Add save/restore functions for UART control regs

login
register
mail settings
Submitter Dirk Behme
Date Dec. 18, 2011, 5:34 p.m.
Message ID <1324229655-5538-1-git-send-email-dirk.behme@gmail.com>
Download mbox | patch
Permalink /patch/132105/
State New
Headers show

Comments

Dirk Behme - Dec. 18, 2011, 5:34 p.m.
Factor out the uart save/restore functionality instead of
having the same code several times in the driver.

Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
CC: Saleem Abdulrasool <compnerd@compnerd.org>
CC: Sascha Hauer <s.hauer@pengutronix.de>
CC: Fabio Estevam <festevam@gmail.com>
CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
CC: linux-serial@vger.kernel.org
---
 drivers/tty/serial/imx.c |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)
Shawn Guo - Dec. 19, 2011, 3:49 a.m.
On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote:
> Factor out the uart save/restore functionality instead of
> having the same code several times in the driver.
> 
> Signed-off-by: Dirk Behme <dirk.behme@gmail.com>
> CC: Saleem Abdulrasool <compnerd@compnerd.org>
> CC: Sascha Hauer <s.hauer@pengutronix.de>
> CC: Fabio Estevam <festevam@gmail.com>
> CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> CC: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c |   38 +++++++++++++++++++++++++++++++-------
>  1 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 163fc90..6a01c2a 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport)
>  }
>  
>  /*
> + * Save and restore functions for UCR1, UCR2 and UCR3 registers
> + */
> +static void imx_console_mode(struct uart_port *port,

Function name imx_console_mode seems not like a couple with
imx_console_restore.  And I guess something like
imx_port_ucrs_save[restore] would be better?

> +			     unsigned int *ucr1,
> +			     unsigned int *ucr2,
> +			     unsigned int *ucr3)

Can we define something like 'struct imx_port_ucrs' to contains these?

Regards,
Shawn

> +{
> +	/* save control registers */
> +	*ucr1 = readl(port->membase + UCR1);
> +	*ucr2 = readl(port->membase + UCR2);
> +	*ucr3 = readl(port->membase + UCR3);
> +}
> +
> +static void imx_console_restore(struct uart_port *port,
> +				unsigned int ucr1,
> +				unsigned int ucr2,
> +				unsigned int ucr3)
> +{
> +	/* restore control registers */
> +	writel(ucr1, port->membase + UCR1);
> +	writel(ucr2, port->membase + UCR2);
> +	writel(ucr3, port->membase + UCR3);
> +}
> +
> +/*
>   * Handle any change of modem status signal since we were last called.
>   */
>  static void imx_mctrl_check(struct imx_port *sport)
> @@ -1118,13 +1143,13 @@ static void
>  imx_console_write(struct console *co, const char *s, unsigned int count)
>  {
>  	struct imx_port *sport = imx_ports[co->index];
> -	unsigned int old_ucr1, old_ucr2, ucr1;
> +	unsigned int old_ucr1, old_ucr2, old_ucr3, ucr1;
>  
>  	/*
> -	 *	First, save UCR1/2 and then disable interrupts
> +	 *	First, save UCR1/2/3 and then disable interrupts
>  	 */
> -	ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
> -	old_ucr2 = readl(sport->port.membase + UCR2);
> +	imx_console_mode(&sport->port, &old_ucr1, &old_ucr2, &old_ucr3);
> +	ucr1 = old_ucr1;
>  
>  	if (is_imx1_uart(sport))
>  		ucr1 |= IMX1_UCR1_UARTCLKEN;
> @@ -1139,12 +1164,11 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
>  
>  	/*
>  	 *	Finally, wait for transmitter to become empty
> -	 *	and restore UCR1/2
> +	 *	and restore UCR1/2/3
>  	 */
>  	while (!(readl(sport->port.membase + USR2) & USR2_TXDC));
>  
> -	writel(old_ucr1, sport->port.membase + UCR1);
> -	writel(old_ucr2, sport->port.membase + UCR2);
> +	imx_console_restore(&sport->port, old_ucr1, old_ucr2, old_ucr3);
>  }
>  
>  /*
> -- 
> 1.7.7.4
Dirk Behme - Dec. 19, 2011, 6:49 a.m.
On 19.12.2011 04:49, Shawn Guo wrote:
> On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote:
>> Factor out the uart save/restore functionality instead of
>> having the same code several times in the driver.
>>
>> Signed-off-by: Dirk Behme<dirk.behme@gmail.com>
>> CC: Saleem Abdulrasool<compnerd@compnerd.org>
>> CC: Sascha Hauer<s.hauer@pengutronix.de>
>> CC: Fabio Estevam<festevam@gmail.com>
>> CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de>
>> CC: linux-serial@vger.kernel.org
>> ---
>>   drivers/tty/serial/imx.c |   38 +++++++++++++++++++++++++++++++-------
>>   1 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 163fc90..6a01c2a 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport)
>>   }
>>
>>   /*
>> + * Save and restore functions for UCR1, UCR2 and UCR3 registers
>> + */
>> +static void imx_console_mode(struct uart_port *port,
>
> Function name imx_console_mode seems not like a couple with
> imx_console_restore.  And I guess something like
> imx_port_ucrs_save[restore] would be better?
>
>> +			     unsigned int *ucr1,
>> +			     unsigned int *ucr2,
>> +			     unsigned int *ucr3)
>
> Can we define something like 'struct imx_port_ucrs' to contains these?

Yes, we could have different function names above and use a struct.

I implemented what Sascha asked for in

http://www.spinics.net/lists/arm-kernel/msg144960.html

though:

-- Sascha wrote: --
I'm thinking about:

imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2);
imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3);
-- Sascha end --

It seems Shawn proposes

imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs);
imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs);

(?)

Sascha, Shawn: Could you agree on what we should use?

Once you agreed, I will send an update of both two patches.

Many thanks and best regards

Dirk
Shawn Guo - Dec. 19, 2011, 7:20 a.m.
On Mon, Dec 19, 2011 at 07:49:56AM +0100, Dirk Behme wrote:
> On 19.12.2011 04:49, Shawn Guo wrote:
> >On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote:
> >>Factor out the uart save/restore functionality instead of
> >>having the same code several times in the driver.
> >>
> >>Signed-off-by: Dirk Behme<dirk.behme@gmail.com>
> >>CC: Saleem Abdulrasool<compnerd@compnerd.org>
> >>CC: Sascha Hauer<s.hauer@pengutronix.de>
> >>CC: Fabio Estevam<festevam@gmail.com>
> >>CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de>
> >>CC: linux-serial@vger.kernel.org
> >>---
> >>  drivers/tty/serial/imx.c |   38 +++++++++++++++++++++++++++++++-------
> >>  1 files changed, 31 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >>index 163fc90..6a01c2a 100644
> >>--- a/drivers/tty/serial/imx.c
> >>+++ b/drivers/tty/serial/imx.c
> >>@@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport)
> >>  }
> >>
> >>  /*
> >>+ * Save and restore functions for UCR1, UCR2 and UCR3 registers
> >>+ */
> >>+static void imx_console_mode(struct uart_port *port,
> >
> >Function name imx_console_mode seems not like a couple with
> >imx_console_restore.  And I guess something like
> >imx_port_ucrs_save[restore] would be better?
> >
> >>+			     unsigned int *ucr1,
> >>+			     unsigned int *ucr2,
> >>+			     unsigned int *ucr3)
> >
> >Can we define something like 'struct imx_port_ucrs' to contains these?
> 
> Yes, we could have different function names above and use a struct.
> 
> I implemented what Sascha asked for in
> 
> http://www.spinics.net/lists/arm-kernel/msg144960.html
> 
> though:
> 
> -- Sascha wrote: --
> I'm thinking about:
> 
> imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2);
> imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3);
> -- Sascha end --
> 
> It seems Shawn proposes
> 
> imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs);
> imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs);
> 
> (?)
> 
> Sascha, Shawn: Could you agree on what we should use?
> 
> Once you agreed, I will send an update of both two patches.
> 
Please Cc Alan Cox <alan@linux.intel.com> for these patches.  Though we
could possibly send them through arm-soc tree, we need to ask for his ack.
Sascha Hauer - Dec. 19, 2011, 10:10 a.m.
On Mon, Dec 19, 2011 at 07:49:56AM +0100, Dirk Behme wrote:
> On 19.12.2011 04:49, Shawn Guo wrote:
> >On Sun, Dec 18, 2011 at 06:34:14PM +0100, Dirk Behme wrote:
> >>Factor out the uart save/restore functionality instead of
> >>having the same code several times in the driver.
> >>
> >>Signed-off-by: Dirk Behme<dirk.behme@gmail.com>
> >>CC: Saleem Abdulrasool<compnerd@compnerd.org>
> >>CC: Sascha Hauer<s.hauer@pengutronix.de>
> >>CC: Fabio Estevam<festevam@gmail.com>
> >>CC: Uwe Kleine-König<u.kleine-koenig@pengutronix.de>
> >>CC: linux-serial@vger.kernel.org
> >>---
> >>  drivers/tty/serial/imx.c |   38 +++++++++++++++++++++++++++++++-------
> >>  1 files changed, 31 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >>index 163fc90..6a01c2a 100644
> >>--- a/drivers/tty/serial/imx.c
> >>+++ b/drivers/tty/serial/imx.c
> >>@@ -260,6 +260,31 @@ static inline int is_imx21_uart(struct imx_port *sport)
> >>  }
> >>
> >>  /*
> >>+ * Save and restore functions for UCR1, UCR2 and UCR3 registers
> >>+ */
> >>+static void imx_console_mode(struct uart_port *port,
> >
> >Function name imx_console_mode seems not like a couple with
> >imx_console_restore.  And I guess something like
> >imx_port_ucrs_save[restore] would be better?
> >
> >>+			     unsigned int *ucr1,
> >>+			     unsigned int *ucr2,
> >>+			     unsigned int *ucr3)
> >
> >Can we define something like 'struct imx_port_ucrs' to contains these?
> 
> Yes, we could have different function names above and use a struct.
> 
> I implemented what Sascha asked for in
> 
> http://www.spinics.net/lists/arm-kernel/msg144960.html
> 
> though:
> 
> -- Sascha wrote: --
> I'm thinking about:
> 
> imx_console_mode(struct uart_port *port, u32 *ucr1, u32 *ucr2, u32 *ucr2);
> imx_console_restore(struct uart_port *port, u32 ucr1, u32 ucr2, u32 ucr3);
> -- Sascha end --
> 
> It seems Shawn proposes
> 
> imx_port_ucrs_save(struct uart_port *port, struct *imx_port_ucrs);
> imx_port_ucrs_restore(struct uart_port *port, struct *imx_port_ucrs);
> 
> (?)
> 
> Sascha, Shawn: Could you agree on what we should use?

I think save/restore are better names.
Maybe I thought about a function which saves the current values *and*
sets ucrx to other values. In that case imx_port_ucrs_save() would be
the wrong name.

> 
> Once you agreed, I will send an update of both two patches.

+1 for save/restore

Sascha

Patch

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 163fc90..6a01c2a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -260,6 +260,31 @@  static inline int is_imx21_uart(struct imx_port *sport)
 }
 
 /*
+ * Save and restore functions for UCR1, UCR2 and UCR3 registers
+ */
+static void imx_console_mode(struct uart_port *port,
+			     unsigned int *ucr1,
+			     unsigned int *ucr2,
+			     unsigned int *ucr3)
+{
+	/* save control registers */
+	*ucr1 = readl(port->membase + UCR1);
+	*ucr2 = readl(port->membase + UCR2);
+	*ucr3 = readl(port->membase + UCR3);
+}
+
+static void imx_console_restore(struct uart_port *port,
+				unsigned int ucr1,
+				unsigned int ucr2,
+				unsigned int ucr3)
+{
+	/* restore control registers */
+	writel(ucr1, port->membase + UCR1);
+	writel(ucr2, port->membase + UCR2);
+	writel(ucr3, port->membase + UCR3);
+}
+
+/*
  * Handle any change of modem status signal since we were last called.
  */
 static void imx_mctrl_check(struct imx_port *sport)
@@ -1118,13 +1143,13 @@  static void
 imx_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct imx_port *sport = imx_ports[co->index];
-	unsigned int old_ucr1, old_ucr2, ucr1;
+	unsigned int old_ucr1, old_ucr2, old_ucr3, ucr1;
 
 	/*
-	 *	First, save UCR1/2 and then disable interrupts
+	 *	First, save UCR1/2/3 and then disable interrupts
 	 */
-	ucr1 = old_ucr1 = readl(sport->port.membase + UCR1);
-	old_ucr2 = readl(sport->port.membase + UCR2);
+	imx_console_mode(&sport->port, &old_ucr1, &old_ucr2, &old_ucr3);
+	ucr1 = old_ucr1;
 
 	if (is_imx1_uart(sport))
 		ucr1 |= IMX1_UCR1_UARTCLKEN;
@@ -1139,12 +1164,11 @@  imx_console_write(struct console *co, const char *s, unsigned int count)
 
 	/*
 	 *	Finally, wait for transmitter to become empty
-	 *	and restore UCR1/2
+	 *	and restore UCR1/2/3
 	 */
 	while (!(readl(sport->port.membase + USR2) & USR2_TXDC));
 
-	writel(old_ucr1, sport->port.membase + UCR1);
-	writel(old_ucr2, sport->port.membase + UCR2);
+	imx_console_restore(&sport->port, old_ucr1, old_ucr2, old_ucr3);
 }
 
 /*