Skip to contentSkip to conversation

serial: imx: enable the clocks only when the uart is used

PATCH Superseded Huang Shijie 6 participants 14 messages Jun 4, 2013 – Jun 10, 2013

Subsystems (2)
Arm i.MX and Layerscape TTY layer and serial drivers
lore
Share & tools
lore

Huang Shijie

author
#0 lore raw

PATCH v2 1/1 serial: imx: enable the clocks only when the uart is used

Current code opens the clocks when the uart driver is probed. This will wastes some power if several uarts are enabled, but not really used.

So close these clocks for uart, and enable the clocks only when the uart is used.

Signed-off-by Huang Shijie <[email protected]>

---
v1 --> v2:
check the return value of clk_prepare_enable().

---
 drivers/tty/serial/imx.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +14 -3
index 72bc1db..7cc4810 100644
@@ -699,6 +699,14 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
+	retval = clk_prepare_enable(sport->clk_per);
+	if (retval)
+		goto error_out1;
+
+	retval = clk_prepare_enable(sport->clk_ipg);
+	if (retval)
+		goto error_out1;
+
 	imx_setup_ufcr(sport, 0);
 
 	/* disable the DREN bit (Data Ready interrupt enable) before

@@ -884,6 +892,9 @@ static void imx_shutdown(struct uart_port *port)
 
 	writel(temp, sport->port.membase + UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
+
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
 }
 
 static void

@@ -1557,6 +1568,9 @@ static int serial_imx_probe(struct platform_device *pdev)
 		goto deinit;
 	platform_set_drvdata(pdev, sport);
 
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
+
 	return 0;
 deinit:
 	if (pdata && pdata->exit)

@@ -1578,9 +1592,6 @@ static int serial_imx_remove(struct platform_device *pdev)
 
 	uart_remove_one_port(&imx_reg, &sport->port);
 
-	clk_disable_unprepare(sport->clk_per);
-	clk_disable_unprepare(sport->clk_ipg);
-
 	if (pdata && pdata->exit)
 		pdata->exit(pdev);
Signature · 1 line
1.7.1
#1 lore raw

PATCH v2 1/1 serial: imx: enable the clocks only when the uart is used

Quoted 8 lines · Huang Shijie

Current code opens the clocks when the uart driver is probed. This will wastes some power if several uarts are enabled, but not really used.

So close these clocks for uart, and enable the clocks only when the uart is used.

Signed-off-by Huang Shijie <[email protected]>

Running linux-next 20130607, I just found that the serial console stops working due to this patch. I think the clocks need to be enabled for console as well.

Shawn

Quoted 65 lines

---
v1 --> v2:
check the return value of clk_prepare_enable().

---
 drivers/tty/serial/imx.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +14 -3
index 72bc1db..7cc4810 100644
@@ -699,6 +699,14 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
+	retval = clk_prepare_enable(sport->clk_per);
+	if (retval)
+		goto error_out1;
+
+	retval = clk_prepare_enable(sport->clk_ipg);
+	if (retval)
+		goto error_out1;
+
 	imx_setup_ufcr(sport, 0);
 
 	/* disable the DREN bit (Data Ready interrupt enable) before

@@ -884,6 +892,9 @@ static void imx_shutdown(struct uart_port *port)
 
 	writel(temp, sport->port.membase + UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
+
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
 }
 
 static void

@@ -1557,6 +1568,9 @@ static int serial_imx_probe(struct platform_device *pdev)
 		goto deinit;
 	platform_set_drvdata(pdev, sport);
 
+	clk_disable_unprepare(sport->clk_per);
+	clk_disable_unprepare(sport->clk_ipg);
+
 	return 0;
 deinit:
 	if (pdata && pdata->exit)

@@ -1578,9 +1592,6 @@ static int serial_imx_remove(struct platform_device *pdev)
 
 	uart_remove_one_port(&imx_reg, &sport->port);
 
-	clk_disable_unprepare(sport->clk_per);
-	clk_disable_unprepare(sport->clk_ipg);
-
 	if (pdata && pdata->exit)
 		pdata->exit(pdev);
Signature · 8 lines
1.7.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author
#2 lore raw

PATCH v2 1/1 serial: imx: enable the clocks only when the uart is used

Quoted 3 lines · Shawn Guo

Running linux-next 20130607, I just found that the serial console stops working due to this patch. I think the clocks need to be enabled for console as well.

could you tell me the board?
i tested on imx6q-sabreauto boards, the console is ok.

thanks
Huang Shijie

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

#3 lore raw

PATCH v2 1/1 serial: imx: enable the clocks only when the uart is used

Quoted 6 lines · Huang Shijie

? 2013?06?08? 14:01, Shawn Guo ??:

Quoted 3 lines

Running linux-next 20130607, I just found that the serial console stops working due to this patch. I think the clocks need to be enabled for console as well.

could you tell me the board?
i tested on imx6q-sabreauto boards, the console is ok.

imx6q-sabresd

Shawn

author
#4 lore raw

PATCH 1/1 serial: imx: enable the clocks for console

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

So enable the clocks for console, and we can see all the log again.

Signed-off-by Huang Shijie <[email protected]>
---
 drivers/tty/serial/imx.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
drivers/tty/serial/imx.c +12 -9
index 7a761f7..b86587f 100644
@@ -702,13 +702,14 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
-	retval = clk_prepare_enable(sport->clk_per);
-	if (retval)
-		goto error_out1;
-
-	retval = clk_prepare_enable(sport->clk_ipg);
-	if (retval)
-		goto error_out1;
+	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)
+			goto error_out1;
+	}
 
 	imx_setup_ufcr(sport, 0);
 

@@ -1578,8 +1579,10 @@ static int serial_imx_probe(struct platform_device *pdev)
 		goto deinit;
 	platform_set_drvdata(pdev, sport);
 
-	clk_disable_unprepare(sport->clk_per);
-	clk_disable_unprepare(sport->clk_ipg);
+	if (!uart_console(&sport->port)) {
+		clk_disable_unprepare(sport->clk_per);
+		clk_disable_unprepare(sport->clk_ipg);
+	}
 
 	return 0;
 deinit:
Signature · 1 line
1.7.1
reviewer
#5 lore raw

PATCH 1/1 serial: imx: enable the clocks for console

Hi Huang,

drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 711

Quoted 7 lines · Huang Shijie
drivers/tty/serial/imx.c +7 -0
+       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)
+                       goto error_out1;

Here you should not jump to "error_out1".

You should jump to a different label and disable the sport->clk_per clock.

This was also wrong in your previous patch.

author
#6 lore raw

PATCH v2 1/1 serial: imx: enable the clocks for console

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

So enable the clocks for console, and we can see all the log again.

This patch also disables the sport->clk_per when we fail to enable the sport->clk_ipg;

Signed-off-by Huang Shijie <[email protected]>

---
v1 --> v2:

	disable the sport->clk_per when we fail to enable
	the sport->clk_ipg;
---
 drivers/tty/serial/imx.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)
drivers/tty/serial/imx.c +14 -9
index 7a761f7..78f8097 100644
@@ -702,13 +702,16 @@ static int imx_startup(struct uart_port *port)
 	int retval;
 	unsigned long flags, temp;
 
-	retval = clk_prepare_enable(sport->clk_per);
-	if (retval)
-		goto error_out1;
-
-	retval = clk_prepare_enable(sport->clk_ipg);
-	if (retval)
-		goto error_out1;
+	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;
+		}
+	}
 
 	imx_setup_ufcr(sport, 0);
 

@@ -1578,8 +1581,10 @@ static int serial_imx_probe(struct platform_device *pdev)
 		goto deinit;
 	platform_set_drvdata(pdev, sport);
 
-	clk_disable_unprepare(sport->clk_per);
-	clk_disable_unprepare(sport->clk_ipg);
+	if (!uart_console(&sport->port)) {
+		clk_disable_unprepare(sport->clk_per);
+		clk_disable_unprepare(sport->clk_ipg);
+	}
 
 	return 0;
 deinit:
Signature · 1 line
1.7.1
#7 lore raw

PATCH v2 1/1 serial: imx: enable the clocks for console

Quoted 10 lines · Huang Shijie

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

So enable the clocks for console, and we can see all the log again.

This patch also disables the sport->clk_per when we fail to enable the sport->clk_ipg;

Signed-off-by Huang Shijie <[email protected]>
Tested-by Shawn Guo <[email protected]>
#8 lore raw

PATCH v2 1/1 serial: imx: enable the clocks for console

Quoted 5 lines · Shawn Guo
Quoted 4 lines · Huang Shijie

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

Quoted 8 lines
Quoted 6 lines

So enable the clocks for console, and we can see all the log again.

This patch also disables the sport->clk_per when we fail to enable the sport->clk_ipg;

Signed-off-by Huang Shijie <[email protected]>
Tested-by Shawn Guo <[email protected]>

Sorry. I have to take that back.

Though it works fine with BusyBox, I'm getting the following warnings when booting with an Ubuntu 13.04 rootfs, in which case imx_startup() and imx_shutdown() will be called for a number of iterations. That causes unbalanced clk_prepare_enable vs. clk_disable_unprepare, since the clk_prepare_enable is under condition if (!uart_console(port)) in imx_startup(), while clk_disable_unprepare in imx_shutdown() is not.

Shawn

------------[ cut here ]------------
WARNING: CPU: 0 PID: 70 at /home/r65073/repos/next/linux-next/drivers/clk/clk.c:780 __clk_disable+0x68/0x84()
Modules linked in:
CPU: 0 PID: 70 Comm: mountall Not tainted 3.10.0-rc4-next-20130607+ #435
Backtrace: 
[<800116a4>] (dump_backtrace+0x0/0x10c) from [<80011844>] (show_stack+0x18/0x1c)
 r6:8069f4e8 r5:0000030c r4:00000000 r3:00000000
[<8001182c>] (show_stack+0x0/0x1c) from [<8053bce0>] (dump_stack+0x78/0x94)
[<8053bc68>] (dump_stack+0x0/0x94) from [<80023df8>] (warn_slowpath_common+0x6c/0x8c)
 r4:00000000 r3:00000000
[<80023d8c>] (warn_slowpath_common+0x0/0x8c) from [<80023e3c>] (warn_slowpath_null+0x24/0x2c)
 r8:bf2ed008 r7:bfaa9810 r6:000f0013 r5:bf824b80 r4:bf824b80
[<80023e18>] (warn_slowpath_null+0x0/0x2c) from [<8041af84>] (__clk_disable+0x68/0x84)
[<8041af1c>] (__clk_disable+0x0/0x84) from [<8041b098>] (clk_disable+0x20/0x2c)
 r4:600f0013 r3:00000001
[<8041b078>] (clk_disable+0x0/0x2c) from [<802c93e8>] (imx_shutdown+0xbc/0xec)
 r5:bf824b80 r4:bfaa9810
[<802c932c>] (imx_shutdown+0x0/0xec) from [<802c63a0>] (uart_port_shutdown+0x34/0x40)
 r5:bf86f860 r4:bfaa9810
[<802c636c>] (uart_port_shutdown+0x0/0x40) from [<802c68c0>] (uart_shutdown+0x98/0xc4)
 r4:bf86f800 r3:00000000
[<802c6828>] (uart_shutdown+0x0/0xc4) from [<802c7514>] (uart_close+0x5c/0x198)
 r7:bfaa9810 r6:bf274400 r5:bf86f86c r4:bf86f800
[<802c74b8>] (uart_close+0x0/0x198) from [<802ac648>] (tty_release+0xf8/0x500)
[<802ac550>] (tty_release+0x0/0x500) from [<800c5a30>] (__fput+0x9c/0x208)
[<800c5994>] (__fput+0x0/0x208) from [<800c5bac>] (____fput+0x10/0x14)
[<800c5b9c>] (____fput+0x0/0x14) from [<80040234>] (task_work_run+0xb4/0xec)
[<80040180>] (task_work_run+0x0/0xec) from [<80029238>] (do_exit+0x2b0/0x920)
 r8:8000e144 r7:000000f8 r6:bf306300 r5:00000000 r4:bfac1180
[<80028f88>] (do_exit+0x0/0x920) from [<80029a4c>] (do_group_exit+0x50/0xd4)
 r7:000000f8
[<800299fc>] (do_group_exit+0x0/0xd4) from [<80029ae8>] (__wake_up_parent+0x0/0x28)
 r7:000000f8 r6:00000001 r5:0006f7ae r4:0006f79a
[<80029ad0>] (SyS_exit_group+0x0/0x18) from [<8000dfc0>] (ret_fast_syscall+0x0/0x30)
---[ end trace 16d080eb7efea4e9 ]---
------------[ cut here ]------------
WARNING: CPU: 0 PID: 70 at /home/r65073/repos/next/linux-next/drivers/clk/clk.c:689 __clk_unprepare+0x6c/0x8c()
Modules linked in:
CPU: 0 PID: 70 Comm: mountall Tainted: G        W    3.10.0-rc4-next-20130607+ #435
Backtrace: 
[<800116a4>] (dump_backtrace+0x0/0x10c) from [<80011844>] (show_stack+0x18/0x1c)
 r6:8069f4e8 r5:000002b1 r4:00000000 r3:00000000
[<8001182c>] (show_stack+0x0/0x1c) from [<8053bce0>] (dump_stack+0x78/0x94)
[<8053bc68>] (dump_stack+0x0/0x94) from [<80023df8>] (warn_slowpath_common+0x6c/0x8c)
 r4:00000000 r3:00000000
[<80023d8c>] (warn_slowpath_common+0x0/0x8c) from [<80023e3c>] (warn_slowpath_null+0x24/0x2c)
 r8:bf2ed008 r7:bfaa9810 r6:000f0013 r5:bf824b80 r4:bf824b80
[<80023e18>] (warn_slowpath_null+0x0/0x2c) from [<8041b68c>] (__clk_unprepare+0x6c/0x8c)
[<8041b620>] (__clk_unprepare+0x0/0x8c) from [<8041b6c8>] (clk_unprepare+0x1c/0x24)
 r4:bf824b80 r3:00000000
[<8041b6ac>] (clk_unprepare+0x0/0x24) from [<802c93f0>] (imx_shutdown+0xc4/0xec)
 r4:bfaa9810 r3:00000000
[<802c932c>] (imx_shutdown+0x0/0xec) from [<802c63a0>] (uart_port_shutdown+0x34/0x40)
 r5:bf86f860 r4:bfaa9810
[<802c636c>] (uart_port_shutdown+0x0/0x40) from [<802c68c0>] (uart_shutdown+0x98/0xc4)
 r4:bf86f800 r3:00000000
[<802c6828>] (uart_shutdown+0x0/0xc4) from [<802c7514>] (uart_close+0x5c/0x198)
 r7:bfaa9810 r6:bf274400 r5:bf86f86c r4:bf86f800
[<802c74b8>] (uart_close+0x0/0x198) from [<802ac648>] (tty_release+0xf8/0x500)
[<802ac550>] (tty_release+0x0/0x500) from [<800c5a30>] (__fput+0x9c/0x208)
[<800c5994>] (__fput+0x0/0x208) from [<800c5bac>] (____fput+0x10/0x14)
[<800c5b9c>] (____fput+0x0/0x14) from [<80040234>] (task_work_run+0xb4/0xec)
[<80040180>] (task_work_run+0x0/0xec) from [<80029238>] (do_exit+0x2b0/0x920)
 r8:8000e144 r7:000000f8 r6:bf306300 r5:00000000 r4:bfac1180
[<80028f88>] (do_exit+0x0/0x920) from [<80029a4c>] (do_group_exit+0x50/0xd4)
 r7:000000f8
[<800299fc>] (do_group_exit+0x0/0xd4) from [<80029ae8>] (__wake_up_parent+0x0/0x28)
 r7:000000f8 r6:00000001 r5:0006f7ae r4:0006f79a
[<80029ad0>] (SyS_exit_group+0x0/0x18) from [<8000dfc0>] (ret_fast_syscall+0x0/0x30)
---[ end trace 16d080eb7efea4ea ]---
------------[ cut here ]------------
maintainer
#9 lore raw

PATCH v2 1/1 serial: imx: enable the clocks for console

Quoted 15 lines · Shawn Guo
Quoted 5 lines · Shawn Guo
Quoted 4 lines · Huang Shijie

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

Quoted 8 lines
Quoted 6 lines

So enable the clocks for console, and we can see all the log again.

This patch also disables the sport->clk_per when we fail to enable the sport->clk_ipg;

Signed-off-by Huang Shijie <[email protected]>
Tested-by Shawn Guo <[email protected]>
Quoted 8 lines

Sorry. I have to take that back.

Though it works fine with BusyBox, I'm getting the following warnings when booting with an Ubuntu 13.04 rootfs, in which case imx_startup() and imx_shutdown() will be called for a number of iterations. That causes unbalanced clk_prepare_enable vs. clk_disable_unprepare, since the clk_prepare_enable is under condition if (!uart_console(port)) in imx_startup(), while clk_disable_unprepare in imx_shutdown() is not.

I had just applied this to my tree, should I revert it because of this problem?

thanks,

greg k-h

#10 lore raw

PATCH v2 1/1 serial: imx: enable the clocks for console

Quoted 25 lines · Greg KH
Quoted 15 lines · Shawn Guo
Quoted 5 lines · Shawn Guo
Quoted 4 lines · Huang Shijie

The console's clocks are disabled after the uart driver is probed. It makes that we can see less log from the console now (though we still can get all the log by the dmesg).

Quoted 8 lines
Quoted 6 lines

So enable the clocks for console, and we can see all the log again.

This patch also disables the sport->clk_per when we fail to enable the sport->clk_ipg;

Signed-off-by Huang Shijie <[email protected]>
Tested-by Shawn Guo <[email protected]>
Quoted 8 lines

Sorry. I have to take that back.

Though it works fine with BusyBox, I'm getting the following warnings when booting with an Ubuntu 13.04 rootfs, in which case imx_startup() and imx_shutdown() will be called for a number of iterations. That causes unbalanced clk_prepare_enable vs. clk_disable_unprepare, since the clk_prepare_enable is under condition if (!uart_console(port)) in imx_startup(), while clk_disable_unprepare in imx_shutdown() is not.

Quoted 2 lines

I had just applied this to my tree, should I revert it because of this problem?

Fabio just sent an incremental fix "serial: imx: Fix serial clock unbalance" for that.

Shawn

author
#11 lore raw

PATCH 1/1 serial: imx: enable the clocks for console

In reply to Fabio Estevam
Quoted 11 lines · Fabio Estevam

Hi Huang,

drivers/tty/serial/imx.c static int imx_startup(struct uart_port *port) · line 711

Quoted 7 lines · Huang Shijie
drivers/tty/serial/imx.c +7 -0
+       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)
+                       goto error_out1;
Quoted 3 lines

Here you should not jump to "error_out1".

You should jump to a different label and disable the sport->clk_per clock.

yes. you are right.
I will fix it in the patch "serial: imx: enable the clocks for console"

thanks
Huang Shijie

Quoted 2 lines

This was also wrong in your previous patch.

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

#12 lore raw
In reply to Shawn Guo
Quoted 9 lines · Huang Shijie
Quoted 6 lines

? 2013?06?08? 14:01, Shawn Guo ??:

Quoted 3 lines

Running linux-next 20130607, I just found that the serial console stops working due to this patch. I think the clocks need to be enabled for console as well.

could you tell me the board?
i tested on imx6q-sabreauto boards, the console is ok.

imx6q-sabresd

I confirm that console not work correct after this patch on the i.MX27 PCM038 board.

---

#13 lore raw

PATCH v2 1/1 serial: imx: enable the clocks only when the uart is used

In reply to Huang Shijie
Quoted 6 lines · Huang Shijie

? 2013?06?08? 14:01, Shawn Guo ??:

Quoted 3 lines

Running linux-next 20130607, I just found that the serial console stops working due to this patch. I think the clocks need to be enabled for console as well.

could you tell me the board?
i tested on imx6q-sabreauto boards, the console is ok.

Take a look at amba-pl011.c to see how to deal with the console and clk API correctly in serial drivers.