From: Huang Shijie <b32955@freescale.com>
To: Shawn Guo <shawn.guo@linaro.org>
Cc: gregkh@linuxfoundation.org, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
fabio.estevam@freescale.com
Subject: Re: [PATCH 1/2] serial: imx: remove the uart_console() check
Date: Fri, 28 Jun 2013 10:17:49 +0800 [thread overview]
Message-ID: <51CCF24D.1060007@freescale.com> (raw)
In-Reply-To: <20130627141535.GD2695@S2101-09.ap.freescale.net>
于 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
WARNING: multiple messages have this Message-ID (diff)
From: b32955@freescale.com (Huang Shijie)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] serial: imx: remove the uart_console() check
Date: Fri, 28 Jun 2013 10:17:49 +0800 [thread overview]
Message-ID: <51CCF24D.1060007@freescale.com> (raw)
In-Reply-To: <20130627141535.GD2695@S2101-09.ap.freescale.net>
? 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
next prev parent reply other threads:[~2013-06-28 2:14 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-26 9:35 [PATCH 1/2] serial: imx: remove the uart_console() check Huang Shijie
2013-06-26 9:35 ` Huang Shijie
2013-06-26 9:35 ` [PATCH 2/2] serial: imx: check the return value when enabling the clocks Huang Shijie
2013-06-26 9:35 ` Huang Shijie
2013-06-27 14:15 ` [PATCH 1/2] serial: imx: remove the uart_console() check Shawn Guo
2013-06-27 14:15 ` Shawn Guo
2013-06-28 2:17 ` Huang Shijie [this message]
2013-06-28 2:17 ` Huang Shijie
2013-06-28 2:55 ` Shawn Guo
2013-06-28 2:55 ` Shawn Guo
2013-06-28 5:53 ` Huang Shijie
2013-06-28 5:53 ` Huang Shijie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51CCF24D.1060007@freescale.com \
--to=b32955@freescale.com \
--cc=fabio.estevam@freescale.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-serial@vger.kernel.org \
--cc=shawn.guo@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.