From: Richard Genoud <richard.genoud@gmail.com> To: David Engraf <david.engraf@sysgo.com>, richard.genoud@gmail.com, gregkh@linuxfoundation.org, jslaby@suse.com, nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com, ludovic.desroches@microchip.com Cc: linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] tty/serial: atmel: fix out of range clock divider handling Date: Fri, 13 Dec 2019 10:57:45 +0100 [thread overview] Message-ID: <822ac68e-4dde-21e8-caf9-a219b910d49e@gmail.com> (raw) In-Reply-To: <20191211162954.8393-1-david.engraf@sysgo.com> Hi, Le 11/12/2019 à 17:29, David Engraf a écrit : > Use MCK_DIV8 when the clock divider is > 65535. Unfortunately the mode > register was already written thus the clock selection is ignored. > > Fix by writing the mode register after calculating the baudrate. > > Signed-off-by: David Engraf <david.engraf@sysgo.com> It seems that this bug was introduced by: commit 5bf5635ac170 ("tty/serial: atmel: add fractional baud rate support") Could you add the "Fixes:" header ? Ludovic, could you check if this was your intent at the time ? > --- > drivers/tty/serial/atmel_serial.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index a8dc8af83f39..9983e2fabbac 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2270,9 +2270,6 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > mode |= ATMEL_US_USMODE_NORMAL; > } > I think it's better to mo move the "Set baud rate" block here (cf bellow) > - /* set the mode, clock divisor, parity, stop bits and data size */ > - atmel_uart_writel(port, ATMEL_US_MR, mode); > - > /* > * when switching the mode, set the RTS line state according to the > * new mode, otherwise keep the former state > @@ -2315,6 +2312,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > } > quot = cd | fp << ATMEL_US_FP_OFFSET; > > + /* set the mode, clock divisor, parity, stop bits and data size */ > + atmel_uart_writel(port, ATMEL_US_MR, mode); > + I think your patch is good, but I'll be happier if instead of moving those 2 lines here, the whole "Set the baud rate" block was moved before "atmel_uart_writel(port, ATMEL_US_MR, mode);" That's because at line 2291 the ATMEL_US_CR register is set with ATMEL_US_RTSDIS or ATMEL_US_RTSEN. And those 2 values have a different effect depending on US_MR.USART_MODE Quoting from the relase manual: RTSEN: 1: Drives RTS pin to 1 if US_MR.USART_MODE field = 2, else drives RTS pin to 0 if US_MR.USART_MODE field = 0. RTSDIS: 1: Drives RTS pin to 0 if US_MR.USART_MODE field = 2, else drives RTS pin to 1 if US_MR.USART_MODE field = 0. So, I think it's better to set the mode register before setting the control register. > if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) > atmel_uart_writel(port, ATMEL_US_BRGR, quot); > atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX); > Thanks !
WARNING: multiple messages have this Message-ID (diff)
From: Richard Genoud <richard.genoud@gmail.com> To: David Engraf <david.engraf@sysgo.com>, richard.genoud@gmail.com, gregkh@linuxfoundation.org, jslaby@suse.com, nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com, ludovic.desroches@microchip.com Cc: linux-arm-kernel@lists.infradead.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] tty/serial: atmel: fix out of range clock divider handling Date: Fri, 13 Dec 2019 10:57:45 +0100 [thread overview] Message-ID: <822ac68e-4dde-21e8-caf9-a219b910d49e@gmail.com> (raw) In-Reply-To: <20191211162954.8393-1-david.engraf@sysgo.com> Hi, Le 11/12/2019 à 17:29, David Engraf a écrit : > Use MCK_DIV8 when the clock divider is > 65535. Unfortunately the mode > register was already written thus the clock selection is ignored. > > Fix by writing the mode register after calculating the baudrate. > > Signed-off-by: David Engraf <david.engraf@sysgo.com> It seems that this bug was introduced by: commit 5bf5635ac170 ("tty/serial: atmel: add fractional baud rate support") Could you add the "Fixes:" header ? Ludovic, could you check if this was your intent at the time ? > --- > drivers/tty/serial/atmel_serial.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index a8dc8af83f39..9983e2fabbac 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2270,9 +2270,6 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > mode |= ATMEL_US_USMODE_NORMAL; > } > I think it's better to mo move the "Set baud rate" block here (cf bellow) > - /* set the mode, clock divisor, parity, stop bits and data size */ > - atmel_uart_writel(port, ATMEL_US_MR, mode); > - > /* > * when switching the mode, set the RTS line state according to the > * new mode, otherwise keep the former state > @@ -2315,6 +2312,9 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios, > } > quot = cd | fp << ATMEL_US_FP_OFFSET; > > + /* set the mode, clock divisor, parity, stop bits and data size */ > + atmel_uart_writel(port, ATMEL_US_MR, mode); > + I think your patch is good, but I'll be happier if instead of moving those 2 lines here, the whole "Set the baud rate" block was moved before "atmel_uart_writel(port, ATMEL_US_MR, mode);" That's because at line 2291 the ATMEL_US_CR register is set with ATMEL_US_RTSDIS or ATMEL_US_RTSEN. And those 2 values have a different effect depending on US_MR.USART_MODE Quoting from the relase manual: RTSEN: 1: Drives RTS pin to 1 if US_MR.USART_MODE field = 2, else drives RTS pin to 0 if US_MR.USART_MODE field = 0. RTSDIS: 1: Drives RTS pin to 0 if US_MR.USART_MODE field = 2, else drives RTS pin to 1 if US_MR.USART_MODE field = 0. So, I think it's better to set the mode register before setting the control register. > if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) > atmel_uart_writel(port, ATMEL_US_BRGR, quot); > atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX); > Thanks ! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-12-13 9:57 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-11 16:29 [PATCH] tty/serial: atmel: fix out of range clock divider handling David Engraf 2019-12-11 16:29 ` David Engraf 2019-12-13 9:57 ` Richard Genoud [this message] 2019-12-13 9:57 ` Richard Genoud 2019-12-13 13:49 ` David Engraf 2019-12-13 13:49 ` David Engraf 2019-12-13 14:03 ` [PATCH v2] " David Engraf 2019-12-13 14:03 ` David Engraf 2019-12-13 16:07 ` Greg KH 2019-12-13 16:07 ` Greg KH 2019-12-16 8:34 ` David Engraf 2019-12-16 8:34 ` David Engraf 2019-12-16 8:54 ` [PATCH v3] " David Engraf 2019-12-16 8:54 ` David Engraf 2019-12-16 10:03 ` Richard Genoud 2019-12-16 10:03 ` Richard Genoud 2019-12-16 10:26 ` Ludovic Desroches 2019-12-16 10:26 ` Ludovic Desroches
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=822ac68e-4dde-21e8-caf9-a219b910d49e@gmail.com \ --to=richard.genoud@gmail.com \ --cc=alexandre.belloni@bootlin.com \ --cc=david.engraf@sysgo.com \ --cc=gregkh@linuxfoundation.org \ --cc=jslaby@suse.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=ludovic.desroches@microchip.com \ --cc=nicolas.ferre@microchip.com \ /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: linkBe 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.