| Message ID | 1420708545-6477-1-git-send-email-pramod.gurav@smartplayin.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 01/08/2015 01:15 AM, Pramod Gurav wrote: > drivers/tty/serial/msm_serial.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c > index c88b522..057008d 100644 > --- a/drivers/tty/serial/msm_serial.c > +++ b/drivers/tty/serial/msm_serial.c > @@ -932,27 +932,12 @@ static int __init msm_console_setup(struct console *co, char *options) > if (unlikely(!port->membase)) > return -ENXIO; > > - msm_init_clock(port); > - Hm.. doesn't the console setup happen before the port is opened though? I would think that we need to keep this around so that the clock is actually enabled before we go and write to hardware registers. > if (options) > uart_parse_options(options, &baud, &parity, &bits, &flow); > > bits = 8; > parity = 'n'; > flow = 'n'; I wonder if we should leave this here? Maybe we can rely on the user specifying the right values on the command line? > - msm_write(port, UART_MR2_BITS_PER_CHAR_8 | UART_MR2_STOP_BIT_LEN_ONE, > - UART_MR2); /* 8N1 */ > - > - if (baud < 300 || baud > 115200) > - baud = 115200; > - msm_set_baud_rate(port, baud); > - > - msm_reset(port); > - > - if (msm_port->is_uartdm) { msm_port is unused now. Please remove it in the same patch.
Hi Stephen, On Friday 09 January 2015 03:16 AM, Stephen Boyd wrote: > On 01/08/2015 01:15 AM, Pramod Gurav wrote: >> drivers/tty/serial/msm_serial.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c >> index c88b522..057008d 100644 >> --- a/drivers/tty/serial/msm_serial.c >> +++ b/drivers/tty/serial/msm_serial.c >> @@ -932,27 +932,12 @@ static int __init msm_console_setup(struct console *co, char *options) >> if (unlikely(!port->membase)) >> return -ENXIO; >> >> - msm_init_clock(port); >> - > > Hm.. doesn't the console setup happen before the port is opened though? > I would think that we need to keep this around so that the clock is > actually enabled before we go and write to hardware registers. Thanks. Verified this. Will undo this change. > >> if (options) >> uart_parse_options(options, &baud, &parity, &bits, &flow); >> >> bits = 8; >> parity = 'n'; >> flow = 'n'; > > I wonder if we should leave this here? Maybe we can rely on the user > specifying the right values on the command line? Ok. Will do. > >> - msm_write(port, UART_MR2_BITS_PER_CHAR_8 | UART_MR2_STOP_BIT_LEN_ONE, >> - UART_MR2); /* 8N1 */ >> - >> - if (baud < 300 || baud > 115200) >> - baud = 115200; >> - msm_set_baud_rate(port, baud); >> - >> - msm_reset(port); >> - >> - if (msm_port->is_uartdm) { > > msm_port is unused now. Please remove it in the same patch. Sure. Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, On Friday 09 January 2015 03:16 AM, Stephen Boyd wrote: > On 01/08/2015 01:15 AM, Pramod Gurav wrote: >> drivers/tty/serial/msm_serial.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c >> index c88b522..057008d 100644 >> --- a/drivers/tty/serial/msm_serial.c >> +++ b/drivers/tty/serial/msm_serial.c >> @@ -932,27 +932,12 @@ static int __init msm_console_setup(struct console *co, char *options) >> if (unlikely(!port->membase)) >> return -ENXIO; >> >> - msm_init_clock(port); >> - > > Hm.. doesn't the console setup happen before the port is opened though? > I would think that we need to keep this around so that the clock is > actually enabled before we go and write to hardware registers. > >> if (options) >> uart_parse_options(options, &baud, &parity, &bits, &flow); >> >> bits = 8; >> parity = 'n'; >> flow = 'n'; Checked some of the driver registering console and they mostly have this but this initialization is done when these variables are declared so that user provided setting won't be overwritten. I think we too should follow the same. With these variables initialized when they are declared user's values will be retained. > > I wonder if we should leave this here? Maybe we can rely on the user > specifying the right values on the command line? > >> - msm_write(port, UART_MR2_BITS_PER_CHAR_8 | UART_MR2_STOP_BIT_LEN_ONE, >> - UART_MR2); /* 8N1 */ >> - >> - if (baud < 300 || baud > 115200) >> - baud = 115200; >> - msm_set_baud_rate(port, baud); >> - >> - msm_reset(port); >> - >> - if (msm_port->is_uartdm) { > > msm_port is unused now. Please remove it in the same patch. > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index c88b522..057008d 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -932,27 +932,12 @@ static int __init msm_console_setup(struct console *co, char *options) if (unlikely(!port->membase)) return -ENXIO; - msm_init_clock(port); - if (options) uart_parse_options(options, &baud, &parity, &bits, &flow); bits = 8; parity = 'n'; flow = 'n'; - msm_write(port, UART_MR2_BITS_PER_CHAR_8 | UART_MR2_STOP_BIT_LEN_ONE, - UART_MR2); /* 8N1 */ - - if (baud < 300 || baud > 115200) - baud = 115200; - msm_set_baud_rate(port, baud); - - msm_reset(port); - - if (msm_port->is_uartdm) { - msm_write(port, UART_CR_CMD_PROTECTION_EN, UART_CR); - msm_write(port, UART_CR_TX_ENABLE, UART_CR); - } pr_info("msm_serial: console setup on port #%d\n", port->line);
The changes done here are already being done somewhere else in code. - The function msm_init_clock() is also called while doing msm_startup hence removing. - msm_set_baud_rate() is anyway called in uart_set_options when it calls msm_set_termios(). msm_reset() is called when we change the baud rate. Hence doing away with both of these calls. - CR_CMD_PROTECTION_EN and CR_TX_ENABLE settings are done in msm_set_baud_rate. So do away with this here. Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> --- Changes since v1: - v1 was just removing call to msm_set_baud_rate(). Other code is removed on review comments from Stephen Boyd here [1]. - Removed call to msm_init_clock() as it is called while doing msm_startup Tested on ifc6410 console. [1]: https://patchwork.kernel.org/patch/5527551/ drivers/tty/serial/msm_serial.c | 15 --------------- 1 file changed, 15 deletions(-)