| Message ID | 1372239359-4054-1-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
? 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
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
? 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
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:
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(-)