| Message ID | 1359475526-17523-1-git-send-email-walimisdev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 30, 2013 at 12:05:26AM +0800, Liming Wang wrote: > There seems no need to call clk_enable in every console writing. And we > can move clk_enable to setup function to enable the clock only once. > Also combine the clk_enable and clk_prepare to clk_prepare_enable. NAK. Just Nak.
On Tue, Jan 29, 2013 at 5:05 PM, Liming Wang <walimisdev@gmail.com> wrote: > There seems no need to call clk_enable in every console writing. And we > can move clk_enable to setup function to enable the clock only once. > Also combine the clk_enable and clk_prepare to clk_prepare_enable. > > Signed-off-by: Liming Wang <walimisdev@gmail.com> NAK. Consider the case where the console outputs nothing or a string every 30 minutes. Why should we not gate off the clock between these sporadic prints? Yours, Linus Walleij
Resend to mailling list. 2013/1/30 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Jan 29, 2013 at 5:05 PM, Liming Wang <walimisdev@gmail.com> wrote: > >> There seems no need to call clk_enable in every console writing. And we >> can move clk_enable to setup function to enable the clock only once. >> Also combine the clk_enable and clk_prepare to clk_prepare_enable. >> >> Signed-off-by: Liming Wang <walimisdev@gmail.com> > > NAK. > > Consider the case where the console outputs nothing or > a string every 30 minutes. Why should we not gate off the Yes, it's reasonable. But I haven't see any other serial drivers do that except pl010/pl011. Except the above consideration, is there any reason that we have to do clk_enable in console write? > clock between these sporadic prints? It works fine under the normal condition, but I encountered backtrace while using kgdboc under preempt_rt kernel: # echo "g" >/proc/sysrq-trigger SysRq : DEBUG <3>BUG: sleeping function called from invalid context at kernel/rtmutex.c:646 <3>in_atomic(): 1, irqs_disabled(): 128, pid: 676, name: sh [<80017d64>] (unwind_backtrace+0x0/0x104) from [<8068fc24>] (dump_stack+0x20/0x24) [<8068fc24>] (dump_stack+0x20/0x24) from [<80061130>] (__might_sleep+0xf4/0x108) [<80061130>] (__might_sleep+0xf4/0x108) from [<80698fc4>] (rt_spin_lock+0x2c/0x38) [<80698fc4>] (rt_spin_lock+0x2c/0x38) from [<80027f1c>] (clk_enable+0x2c/0x4c) [<80027f1c>] (clk_enable+0x2c/0x4c) from [<803fddcc>] (pl011_console_write+0x34/0x148) [<803fddcc>] (pl011_console_write+0x34/0x148) from [<8009e60c>] (vkdb_printf+0x384/0xa78) [<8009e60c>] (vkdb_printf+0x384/0xa78) from [<8009ed44>] (kdb_printf+0x44/0x58) [<8009ed44>] (kdb_printf+0x44/0x58) from [<800a399c>] (kdb_main_loop+0x10c/0x914) [<800a399c>] (kdb_main_loop+0x10c/0x914) from [<800a7a34>] (kdb_stub+0x800/0x95c) [<800a7a34>] (kdb_stub+0x800/0x95c) from [<8009b7b0>] (kgdb_cpu_enter+0x350/0x780) [<8009b7b0>] (kgdb_cpu_enter+0x350/0x780) from [<8009be64>] (kgdb_handle_exception+0x94/0x1bc) [<8009be64>] (kgdb_handle_exception+0x94/0x1bc) from [<80016efc>] (kgdb_compiled_brk_fn+0x38/0x40) [<80016efc>] (kgdb_compiled_brk_fn+0x38/0x40) from [<800082f4>] (do_undefinstr+0x154/0x184) [<800082f4>] (do_undefinstr+0x154/0x184) from [<80699a2c>] (__und_svc_finish+0x0/0x14) Exception stack(0xbcd6de08 to 0xbcd6de50) de00: bcd6c000 bcd6c028 bcd6de68 80020318 809b77c0 809b77c8 de20: 80923e5c 00000007 00000000 bcd6c000 bcd6df68 bcd6deac bcd6de38 bcd6de90 de40: 800627d8 8009ac28 600d0013 ffffffff [<80699a2c>] (__und_svc_finish+0x0/0x14) from [<8009ac28>] (kgdb_breakpoint+0x50/0x80) [<8009ac28>] (kgdb_breakpoint+0x50/0x80) from [<8009af18>] (sysrq_handle_dbg+0x4c/0x70) [<8009af18>] (sysrq_handle_dbg+0x4c/0x70) from [<803e9c38>] (__handle_sysrq+0x138/0x19c) [<803e9c38>] (__handle_sysrq+0x138/0x19c) from [<803e9ce4>] (write_sysrq_trigger+0x48/0x58) [<803e9ce4>] (write_sysrq_trigger+0x48/0x58) from [<801886c8>] (proc_reg_write+0x88/0xb0) [<801886c8>] (proc_reg_write+0x88/0xb0) from [<80132b1c>] (vfs_write+0xb8/0x148) [<80132b1c>] (vfs_write+0xb8/0x148) from [<80132e64>] (sys_write+0x50/0x124) [<80132e64>] (sys_write+0x50/0x124) from [<8000e7a0>] (ret_fast_syscall+0x0/0x30) [<80132e64>] (sys_write+0x50/0x124) from [<8on processor 1 due to Keyboard Entry [1]kdb> It seems that clk_enable/clk_disable are called in pl011_console_write function with the irq disabeld. So that spin_lock in clk_enable/clk_disable trips the rtmutex debug code. There are two ways to resolve the issues: 1. One way is to convert spin_lock in clk_enable/clk_disable to raw spin_lock. 2. Another way is to move clk_enable/clk_disable from console write. This patch is an attempt to make clear whether we can move clk_enable/clk_disable from console write. Thanks Liming Wang > > Yours, > Linus Walleij
On Wed, Jan 30, 2013 at 06:41:34PM +0800, walimis wrote: > 2013/1/30 Linus Walleij <linus.walleij@linaro.org>: > > On Tue, Jan 29, 2013 at 5:05 PM, Liming Wang <walimisdev@gmail.com> wrote: > > > >> There seems no need to call clk_enable in every console writing. And we > >> can move clk_enable to setup function to enable the clock only once. > >> Also combine the clk_enable and clk_prepare to clk_prepare_enable. > >> > >> Signed-off-by: Liming Wang <walimisdev@gmail.com> > > > > NAK. > > > > Consider the case where the console outputs nothing or > > a string every 30 minutes. Why should we not gate off the > > Yes, it's reasonable. But I haven't see any other serial drivers do that except > pl010/pl011. Except the above consideration, is there any reason that we have to > do clk_enable in console write? The reason that it's done is because the author of the driver (me) is also the author of the clk API, and I know how these things work, and I know how to code drivers to work most effectively with it. Everyone else appears to be totally and utterly lazy about it. > It works fine under the normal condition, but I encountered backtrace > while using kgdboc under preempt_rt kernel: > > # echo "g" >/proc/sysrq-trigger > SysRq : DEBUG > <3>BUG: sleeping function called from invalid context at kernel/rtmutex.c:646 > <3>in_atomic(): 1, irqs_disabled(): 128, pid: 676, name: sh > [<80017d64>] (unwind_backtrace+0x0/0x104) from [<8068fc24>] > (dump_stack+0x20/0x24) > [<8068fc24>] (dump_stack+0x20/0x24) from [<80061130>] (__might_sleep+0xf4/0x108) > [<80061130>] (__might_sleep+0xf4/0x108) from [<80698fc4>] > (rt_spin_lock+0x2c/0x38) > [<80698fc4>] (rt_spin_lock+0x2c/0x38) from [<80027f1c>] (clk_enable+0x2c/0x4c) Well, I guess that's down to the use of rt_spin_lock there. I think that's one for the RT Preempt guys; I don't deal with RT kernels at all - I know nothing about that. In standard kernel programming it is perfectly acceptable to take spinlocks in non-preemptable contexts like this. > This patch is an attempt to make clear whether we can move > clk_enable/clk_disable from console write. clk_enable/clk_disable should be callable from this context; the bug is that they aren't.
diff --git a/drivers/tty/serial/amba-pl010.c b/drivers/tty/serial/amba-pl010.c index 22317dd..9b77188 100644 --- a/drivers/tty/serial/amba-pl010.c +++ b/drivers/tty/serial/amba-pl010.c @@ -569,8 +569,6 @@ pl010_console_write(struct console *co, const char *s, unsigned int count) struct uart_amba_port *uap = amba_ports[co->index]; unsigned int status, old_cr; - clk_enable(uap->clk); - /* * First save the CR then disable the interrupts */ @@ -588,8 +586,6 @@ pl010_console_write(struct console *co, const char *s, unsigned int count) barrier(); } while (status & UART01x_FR_BUSY); writel(old_cr, uap->port.membase + UART010_CR); - - clk_disable(uap->clk); } static void __init @@ -639,7 +635,7 @@ static int __init pl010_console_setup(struct console *co, char *options) if (!uap) return -ENODEV; - ret = clk_prepare(uap->clk); + ret = clk_prepare_enable(uap->clk); if (ret) return ret; diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 7fca402..53aafd4 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1777,8 +1777,6 @@ pl011_console_write(struct console *co, const char *s, unsigned int count) unsigned long flags; int locked = 1; - clk_enable(uap->clk); - local_irq_save(flags); if (uap->port.sysrq) locked = 0; @@ -1809,8 +1807,6 @@ pl011_console_write(struct console *co, const char *s, unsigned int count) if (locked) spin_unlock(&uap->port.lock); local_irq_restore(flags); - - clk_disable(uap->clk); } static void __init @@ -1876,7 +1872,7 @@ static int __init pl011_console_setup(struct console *co, char *options) "could not set default pins\n"); } - ret = clk_prepare(uap->clk); + ret = clk_prepare_enable(uap->clk); if (ret) return ret;
There seems no need to call clk_enable in every console writing. And we can move clk_enable to setup function to enable the clock only once. Also combine the clk_enable and clk_prepare to clk_prepare_enable. Signed-off-by: Liming Wang <walimisdev@gmail.com> --- drivers/tty/serial/amba-pl010.c | 6 +----- drivers/tty/serial/amba-pl011.c | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-)