[PATCH 1/2] serial: imx: remove the uart_console() check

[PATCH 1/2] serial: imx: remove the uart_console() check

[Huang Shijie]

The uart_console() check makes the clocks(clk_per and clk_ipg) opened
even when we close the console uart.

This patch enable/disable the clocks in imx_console_write(),
so we can keep the clocks closed when the console uart is closed.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/tty/serial/imx.c |   63 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 415cec6..bfdf764 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -702,15 +702,13 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
-	if (!uart_console(port)) {
-		retval = clk_prepare_enable(sport->clk_per);
-		if (retval)
-			goto error_out1;
-		retval = clk_prepare_enable(sport->clk_ipg);
-		if (retval) {
-			clk_disable_unprepare(sport->clk_per);
-			goto error_out1;
-		}
+	retval = clk_prepare_enable(sport->clk_per);
+	if (retval)
+		goto error_out1;
+	retval = clk_prepare_enable(sport->clk_ipg);
+	if (retval) {
+		clk_disable_unprepare(sport->clk_per);
+		goto error_out1;
 	}
 
 	imx_setup_ufcr(sport, 0);
@@ -901,10 +899,8 @@ static void imx_shutdown(struct uart_port *port)
 	writel(temp, sport->port.membase + UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-	if (!uart_console(&sport->port)) {
-		clk_disable_unprepare(sport->clk_per);
-		clk_disable_unprepare(sport->clk_ipg);
-	}
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
 }
 
 static void
@@ -1251,6 +1247,16 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
 	unsigned int ucr1;
 	unsigned long flags = 0;
 	int locked = 1;
+	int retval;
+
+	retval = clk_enable(sport->clk_per);
+	if (retval)
+		return;
+	retval = clk_enable(sport->clk_ipg);
+	if (retval) {
+		clk_disable(sport->clk_per);
+		return;
+	}
 
 	if (sport->port.sysrq)
 		locked = 0;
@@ -1286,6 +1292,9 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
 
 	if (locked)
 		spin_unlock_irqrestore(&sport->port.lock, flags);
+
+	clk_disable(sport->clk_ipg);
+	clk_disable(sport->clk_per);
 }
 
 /*
@@ -1359,6 +1368,7 @@ imx_console_setup(struct console *co, char *options)
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
+	int retval;
 
 	/*
 	 * Check whether an invalid uart number has been specified, and
@@ -1371,6 +1381,15 @@ imx_console_setup(struct console *co, char *options)
 	if (sport == NULL)
 		return -ENODEV;
 
+	retval = clk_prepare_enable(sport->clk_per);
+	if (retval)
+		goto error_console;
+	retval = clk_prepare_enable(sport->clk_ipg);
+	if (retval) {
+		clk_disable_unprepare(sport->clk_per);
+		goto error_console;
+	}
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else
@@ -1378,7 +1397,17 @@ imx_console_setup(struct console *co, char *options)
 
 	imx_setup_ufcr(sport, 0);
 
-	return uart_set_options(&sport->port, co, baud, parity, bits, flow);
+	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
+
+	clk_disable(sport->clk_per);
+	clk_disable(sport->clk_ipg);
+	if (retval) {
+		clk_unprepare(sport->clk_per);
+		clk_unprepare(sport->clk_ipg);
+	}
+
+error_console:
+	return retval;
 }
 
 static struct uart_driver imx_reg;
@@ -1583,10 +1612,8 @@ static int serial_imx_probe(struct platform_device *pdev)
 		goto deinit;
 	platform_set_drvdata(pdev, sport);
 
-	if (!uart_console(&sport->port)) {
-		clk_disable_unprepare(sport->clk_per);
-		clk_disable_unprepare(sport->clk_ipg);
-	}
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
 
 	return 0;
 deinit:
-- 
1.7.1

[PATCH 2/2] serial: imx: check the return value when enabling the clocks

[Huang Shijie]

In the probe function, we do not check the return value of the
clk_prepare_enable(). But in actually, the clk_prepare_enable() may fails,
so check the return value when we enable the clocks.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/tty/serial/imx.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bfdf764..2aec568 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1593,8 +1593,15 @@ static int serial_imx_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	clk_prepare_enable(sport->clk_per);
-	clk_prepare_enable(sport->clk_ipg);
+	ret = clk_prepare_enable(sport->clk_per);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(sport->clk_ipg);
+	if (ret) {
+		clk_disable_unprepare(sport->clk_per);
+		return ret;
+	}
 
 	sport->port.uartclk = clk_get_rate(sport->clk_per);
 
-- 
1.7.1

Re: [PATCH 1/2] serial: imx: remove the uart_console() check

[Shawn Guo]

On Wed, Jun 26, 2013 at 05:35:58PM +0800, Huang Shijie wrote:
> The uart_console() check makes the clocks(clk_per and clk_ipg) opened
> even when we close the console uart.
> 
> This patch enable/disable the clocks in imx_console_write(),
> so we can keep the clocks closed when the console uart is closed.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/tty/serial/imx.c |   63 ++++++++++++++++++++++++++++++++-------------
>  1 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 415cec6..bfdf764 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -702,15 +702,13 @@ static int imx_startup(struct uart_port *port)
>  	int retval;
>  	unsigned long flags, temp;
>  
> -	if (!uart_console(port)) {
> -		retval = clk_prepare_enable(sport->clk_per);
> -		if (retval)
> -			goto error_out1;
> -		retval = clk_prepare_enable(sport->clk_ipg);
> -		if (retval) {
> -			clk_disable_unprepare(sport->clk_per);
> -			goto error_out1;
> -		}
> +	retval = clk_prepare_enable(sport->clk_per);
> +	if (retval)
> +		goto error_out1;
> +	retval = clk_prepare_enable(sport->clk_ipg);
> +	if (retval) {
> +		clk_disable_unprepare(sport->clk_per);
> +		goto error_out1;
>  	}
>  
>  	imx_setup_ufcr(sport, 0);
> @@ -901,10 +899,8 @@ static void imx_shutdown(struct uart_port *port)
>  	writel(temp, sport->port.membase + UCR1);
>  	spin_unlock_irqrestore(&sport->port.lock, flags);
>  
> -	if (!uart_console(&sport->port)) {
> -		clk_disable_unprepare(sport->clk_per);
> -		clk_disable_unprepare(sport->clk_ipg);
> -	}
> +	clk_disable_unprepare(sport->clk_per);
> +	clk_disable_unprepare(sport->clk_ipg);
>  }
>  
>  static void
> @@ -1251,6 +1247,16 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
>  	unsigned int ucr1;
>  	unsigned long flags = 0;
>  	int locked = 1;
> +	int retval;
> +
> +	retval = clk_enable(sport->clk_per);
> +	if (retval)
> +		return;
> +	retval = clk_enable(sport->clk_ipg);
> +	if (retval) {
> +		clk_disable(sport->clk_per);
> +		return;
> +	}
>  
>  	if (sport->port.sysrq)
>  		locked = 0;
> @@ -1286,6 +1292,9 @@ imx_console_write(struct console *co, const char *s, unsigned int count)
>  
>  	if (locked)
>  		spin_unlock_irqrestore(&sport->port.lock, flags);
> +
> +	clk_disable(sport->clk_ipg);
> +	clk_disable(sport->clk_per);
>  }
>  
>  /*
> @@ -1359,6 +1368,7 @@ imx_console_setup(struct console *co, char *options)
>  	int bits = 8;
>  	int parity = 'n';
>  	int flow = 'n';
> +	int retval;
>  
>  	/*
>  	 * Check whether an invalid uart number has been specified, and
> @@ -1371,6 +1381,15 @@ imx_console_setup(struct console *co, char *options)
>  	if (sport == NULL)
>  		return -ENODEV;
>  
> +	retval = clk_prepare_enable(sport->clk_per);
> +	if (retval)
> +		goto error_console;
> +	retval = clk_prepare_enable(sport->clk_ipg);
> +	if (retval) {
> +		clk_disable_unprepare(sport->clk_per);
> +		goto error_console;
> +	}
> +

Why do we need clk_enable() here at all?  The amba-pl011 driver only
calls clk_prepare() in console .setup().

>  	if (options)
>  		uart_parse_options(options, &baud, &parity, &bits, &flow);
>  	else
> @@ -1378,7 +1397,17 @@ imx_console_setup(struct console *co, char *options)
>  
>  	imx_setup_ufcr(sport, 0);
>  
> -	return uart_set_options(&sport->port, co, baud, parity, bits, flow);
> +	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
> +
> +	clk_disable(sport->clk_per);
> +	clk_disable(sport->clk_ipg);
> +	if (retval) {
> +		clk_unprepare(sport->clk_per);
> +		clk_unprepare(sport->clk_ipg);
> +	}
> +
> +error_console:
> +	return retval;
>  }
>  
>  static struct uart_driver imx_reg;
> @@ -1583,10 +1612,8 @@ static int serial_imx_probe(struct platform_device *pdev)
>  		goto deinit;
>  	platform_set_drvdata(pdev, sport);
>  
> -	if (!uart_console(&sport->port)) {
> -		clk_disable_unprepare(sport->clk_per);
> -		clk_disable_unprepare(sport->clk_ipg);
> -	}
> +	clk_disable_unprepare(sport->clk_per);
> +	clk_disable_unprepare(sport->clk_ipg);

I also had a hard time to understand why we need to turn on the clocks
in .probe() for a while and then turn them off.

It just reminds me a thing.  Did you test CONFIG_CONSOLE_POLL support
when your commit 28eb427 (serial: imx: enable the clocks only when the
uart is used) went in?  The commit turns off the clocks at the
end of .probe(), but who will enable the clocks for .poll_get_char()
and .poll_put_char()?  The amba-pl011 driver does that in .poll_init().

Shawn

>  
>  	return 0;
>  deinit:
> -- 
> 1.7.1
> 
> 

Re: [PATCH 1/2] serial: imx: remove the uart_console() check

[Huang Shijie]

于 2013年06月27日 22:15, Shawn Guo 写道:
>>   	* Check whether an invalid uart number has been specified, and
>> >  @@ -1371,6 +1381,15 @@ imx_console_setup(struct console *co, char *options)
>> >    	if (sport == NULL)
>> >    		return -ENODEV;
>> >  
>> >  +	retval = clk_prepare_enable(sport->clk_per);
>> >  +	if (retval)
>> >  +		goto error_console;
>> >  +	retval = clk_prepare_enable(sport->clk_ipg);
>> >  +	if (retval) {
>> >  +		clk_disable_unprepare(sport->clk_per);
>> >  +		goto error_console;
>> >  +	}
>> >  +
> Why do we need clk_enable() here at all?  The amba-pl011 driver only
> calls clk_prepare() in console .setup().
>
We need to set the imx_setUp_ufcr() in our imx_console_setup(),
so we need to enable the clocks, aren't we?
>> >    	if (options)
>> >    		uart_parse_options(options,&baud,&parity,&bits,&flow);
>> >    	else
>> >  @@ -1378,7 +1397,17 @@ imx_console_setup(struct console *co, char *options)
>> >  
>> >    	imx_setup_ufcr(sport, 0);
>> >  
>> >  -	return uart_set_options(&sport->port, co, baud, parity, bits, flow);
>> >  +	retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
>> >  +
>> >  +	clk_disable(sport->clk_per);
>> >  +	clk_disable(sport->clk_ipg);
>> >  +	if (retval) {
>> >  +		clk_unprepare(sport->clk_per);
>> >  +		clk_unprepare(sport->clk_ipg);
>> >  +	}
>> >  +
>> >  +error_console:
>> >  +	return retval;
>> >    }
>> >  
>> >    static struct uart_driver imx_reg;
>> >  @@ -1583,10 +1612,8 @@ static int serial_imx_probe(struct platform_device *pdev)
>> >    		goto deinit;
>> >    	platform_set_drvdata(pdev, sport);
>> >  
>> >  -	if (!uart_console(&sport->port)) {
>> >  -		clk_disable_unprepare(sport->clk_per);
>> >  -		clk_disable_unprepare(sport->clk_ipg);
>> >  -	}
>> >  +	clk_disable_unprepare(sport->clk_per);
>> >  +	clk_disable_unprepare(sport->clk_ipg);
> I also had a hard time to understand why we need to turn on the clocks
> in .probe() for a while and then turn them off.
In the probe's uart_add_one_port(), we will register the console and 
call the setup() hook,
so it's ok to disable the clocks in the end of the probe.
> It just reminds me a thing.  Did you test CONFIG_CONSOLE_POLL support
> when your commit 28eb427 (serial: imx: enable the clocks only when the
> uart is used) went in?  The commit turns off the clocks at the
sorry, i did not do this.
> end of .probe(), but who will enable the clocks for .poll_get_char()
> and .poll_put_char()?  The amba-pl011 driver does that in .poll_init().
>
dido. in the uart_add_one_port().

thanks
Huang Shijie


--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] serial: imx: remove the uart_console() check

[Shawn Guo]

On Fri, Jun 28, 2013 at 10:17:49AM +0800, Huang Shijie wrote:
> We need to set the imx_setUp_ufcr() in our imx_console_setup(),
> so we need to enable the clocks, aren't we?

Ah, yes, I missed that.  But register access only needs ipg clock and
per clock still does not need to be enabled here, right?

> In the probe's uart_add_one_port(), we will register the console and
> call the setup() hook,
> so it's ok to disable the clocks in the end of the probe.

Look, here is what you do in .probe().

	clk_prepare_enable(sport->clk_per);
	clk_prepare_enable(sport->clk_ipg); 
	...
	uart_add_one_port(&imx_reg, &sport->port);
	...
	clk_disable_unprepare(sport->clk_per);
	clk_disable_unprepare(sport->clk_ipg);

Since imx_console_setup() will be called in uart_add_one_port() and
clocks are already being taken care of in imx_console_setup(), why do
you need all these clock operations here at all?

Shawn

Re: [PATCH 1/2] serial: imx: remove the uart_console() check

[Huang Shijie]

于 2013年06月28日 10:55, Shawn Guo 写道:
> On Fri, Jun 28, 2013 at 10:17:49AM +0800, Huang Shijie wrote:
>> We need to set the imx_setUp_ufcr() in our imx_console_setup(),
>> so we need to enable the clocks, aren't we?
> Ah, yes, I missed that.  But register access only needs ipg clock and
> per clock still does not need to be enabled here, right?
yes. we can only enable the ipg clock.
>> In the probe's uart_add_one_port(), we will register the console and
>> call the setup() hook,
>> so it's ok to disable the clocks in the end of the probe.
> Look, here is what you do in .probe().
>
> 	clk_prepare_enable(sport->clk_per);
> 	clk_prepare_enable(sport->clk_ipg);
> 	...
> 	uart_add_one_port(&imx_reg,&sport->port);
> 	...
> 	clk_disable_unprepare(sport->clk_per);
> 	clk_disable_unprepare(sport->clk_ipg);
>
> Since imx_console_setup() will be called in uart_add_one_port() and
> clocks are already being taken care of in imx_console_setup(), why do
> you need all these clock operations here at all?

I will remove all the clock operations in the probe.

thanks for pointing this.


Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html