* commit 5fe212364 causes division by zero with large bauds @ 2013-09-10 19:09 Felipe Balbi 2013-09-11 6:22 ` Alexey Pelykh 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2013-09-10 19:09 UTC (permalink / raw) To: Alexey Pelykh Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 4409 bytes --] Hi Alexey, your commit 5fe212364 causes division by zero with any baud rate larger than 3 Mbaud (IP supports up to 3686400). Maybe this patch is all we need to get it fixed ? (untested) diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 816d1a2..b50327f 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -240,8 +240,14 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) { unsigned int n13 = port->uartclk / (13 * baud); unsigned int n16 = port->uartclk / (16 * baud); - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); + int baudAbsDiff13 = 0; + int baudAbsDiff16 = 0; + + if (n13) + baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); + if (n16) + baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); + if(baudAbsDiff13 < 0) baudAbsDiff13 = -baudAbsDiff13; if(baudAbsDiff16 < 0) Another possibility would be to convert this into a lookup table (also untested): diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 816d1a2..942d68e 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -224,6 +224,48 @@ static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable) pdata->enable_wakeup(up->dev, enable); } +struct uart_omap_config { + unsigned int baud; + unsigned int oversampling; + unsigned int divisor; +}; + +static struct uart_omap_config omap_port_configs[] = { + { .baud = 300, .oversampling = 16, .divisor = 10000, }, + { .baud = 300, .oversampling = 16, .divisor = 10000, }, + { .baud = 600, .oversampling = 16, .divisor = 5000, }, + { .baud = 1200, .oversampling = 16, .divisor = 2500, }, + { .baud = 2400, .oversampling = 16, .divisor = 1250, }, + { .baud = 4800, .oversampling = 16, .divisor = 625, }, + { .baud = 9600, .oversampling = 16, .divisor = 312, }, + { .baud = 14400, .oversampling = 16, .divisor = 208, }, + { .baud = 19200, .oversampling = 16, .divisor = 156, }, + { .baud = 28800, .oversampling = 16, .divisor = 704, }, + { .baud = 38400, .oversampling = 16, .divisor = 78, }, + { .baud = 57600, .oversampling = 16, .divisor = 52, }, + { .baud = 115200, .oversampling = 16, .divisor = 26, }, + { .baud = 230400, .oversampling = 16, .divisor = 13, }, + { .baud = 460800, .oversampling = 13, .divisor = 8, }, + { .baud = 921600, .oversampling = 13, .divisor = 4, }, + { .baud = 1843200, .oversampling = 13, .divisor = 2, }, + { .baud = 3000000, .oversampling = 16, .divisor = 1, }, + { .baud = 3686400, .oversampling = 13, .divisor = 1, }, + { } /* Terminating Entry */ +}; + +static struct uart_omap_config * +__serial_omap_get_config(struct uart_port *port, unsigned int baud) +{ + struct uart_omap_config *cfg = omap_port_configs; + + while (cfg++) { + if (cfg->baud == baud) + return cfg; + } + + return NULL; +} + /* * serial_omap_baud_is_mode16 - check if baud rate is MODE16X * @port: uart port info @@ -238,16 +280,12 @@ static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable) static bool serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) { - unsigned int n13 = port->uartclk / (13 * baud); - unsigned int n16 = port->uartclk / (16 * baud); - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); - if(baudAbsDiff13 < 0) - baudAbsDiff13 = -baudAbsDiff13; - if(baudAbsDiff16 < 0) - baudAbsDiff16 = -baudAbsDiff16; - - return (baudAbsDiff13 > baudAbsDiff16); + struct uart_omap_config *cfg = __serial_omap_get_config(port, baud); + + if (!cfg) + return -EINVAL; + + return (cfg->oversampling == 16); } /* @@ -258,13 +296,12 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) static unsigned int serial_omap_get_divisor(struct uart_port *port, unsigned int baud) { - unsigned int divisor; + struct uart_omap_config *cfg = __serial_omap_get_config(port, baud); - if (!serial_omap_baud_is_mode16(port, baud)) - divisor = 13; - else - divisor = 16; - return port->uartclk/(baud * divisor); + if (!cfg) + return -EINVAL; + + return cfg->divisor; } static void serial_omap_enable_ms(struct uart_port *port) -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-10 19:09 commit 5fe212364 causes division by zero with large bauds Felipe Balbi @ 2013-09-11 6:22 ` Alexey Pelykh 2013-09-11 18:38 ` Felipe Balbi 0 siblings, 1 reply; 12+ messages in thread From: Alexey Pelykh @ 2013-09-11 6:22 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List Hi Felipe, Thanks for finding this issue. Indeed, there is a bug on 3M+ baud rates. First patch is close to a complete fix, but still contains div-by-zero issue. Here is my version: diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 816d1a2..808a880 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) { unsigned int n13 = port->uartclk / (13 * baud); unsigned int n16 = port->uartclk / (16 * baud); - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; if(baudAbsDiff13 < 0) baudAbsDiff13 = -baudAbsDiff13; if(baudAbsDiff16 < 0) With 48MHz UART clock, it will give 300: divisor = 12307 (13), real rate 300 (0.000000%) 600: divisor = 6153 (13), real rate 600 (0.000000%) 1200: divisor = 3076 (13), real rate 1200 (0.000000%) 2400: divisor = 1538 (13), real rate 2400 (0.000000%) 4800: divisor = 625 (16), real rate 4800 (0.000000%) 9600: divisor = 384 (13), real rate 9615 (0.156250%) 14400: divisor = 256 (13), real rate 14423 (0.159722%) 19200: divisor = 192 (13), real rate 19230 (0.156250%) 28800: divisor = 128 (13), real rate 28846 (0.159722%) 38400: divisor = 96 (13), real rate 38461 (0.158854%) 57600: divisor = 64 (13), real rate 57692 (0.159722%) 115200: divisor = 32 (13), real rate 115384 (0.159722%) 230400: divisor = 16 (13), real rate 230769 (0.160156%) 460800: divisor = 8 (13), real rate 461538 (0.160156%) 921600: divisor = 4 (13), real rate 923076 (0.160156%) 1000000: divisor = 3 (16), real rate 1000000 (0.000000%) 1843200: divisor = 2 (13), real rate 1846153 (0.160211%) 3000000: divisor = 1 (16), real rate 3000000 (0.000000%) 3686400: divisor = 1 (13), real rate 3692307 (0.160238%) Thanks, Alexey On Tue, Sep 10, 2013 at 10:09 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi Alexey, > > your commit 5fe212364 causes division by zero with any baud rate larger > than 3 Mbaud (IP supports up to 3686400). > > Maybe this patch is all we need to get it fixed ? (untested) > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 816d1a2..b50327f 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -240,8 +240,14 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) > { > unsigned int n13 = port->uartclk / (13 * baud); > unsigned int n16 = port->uartclk / (16 * baud); > - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > + int baudAbsDiff13 = 0; > + int baudAbsDiff16 = 0; > + > + if (n13) > + baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > + if (n16) > + baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > + > if(baudAbsDiff13 < 0) > baudAbsDiff13 = -baudAbsDiff13; > if(baudAbsDiff16 < 0) > > Another possibility would be to convert this into a lookup table (also > untested): > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 816d1a2..942d68e 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -224,6 +224,48 @@ static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable) > pdata->enable_wakeup(up->dev, enable); > } > > +struct uart_omap_config { > + unsigned int baud; > + unsigned int oversampling; > + unsigned int divisor; > +}; > + > +static struct uart_omap_config omap_port_configs[] = { > + { .baud = 300, .oversampling = 16, .divisor = 10000, }, > + { .baud = 300, .oversampling = 16, .divisor = 10000, }, > + { .baud = 600, .oversampling = 16, .divisor = 5000, }, > + { .baud = 1200, .oversampling = 16, .divisor = 2500, }, > + { .baud = 2400, .oversampling = 16, .divisor = 1250, }, > + { .baud = 4800, .oversampling = 16, .divisor = 625, }, > + { .baud = 9600, .oversampling = 16, .divisor = 312, }, > + { .baud = 14400, .oversampling = 16, .divisor = 208, }, > + { .baud = 19200, .oversampling = 16, .divisor = 156, }, > + { .baud = 28800, .oversampling = 16, .divisor = 704, }, > + { .baud = 38400, .oversampling = 16, .divisor = 78, }, > + { .baud = 57600, .oversampling = 16, .divisor = 52, }, > + { .baud = 115200, .oversampling = 16, .divisor = 26, }, > + { .baud = 230400, .oversampling = 16, .divisor = 13, }, > + { .baud = 460800, .oversampling = 13, .divisor = 8, }, > + { .baud = 921600, .oversampling = 13, .divisor = 4, }, > + { .baud = 1843200, .oversampling = 13, .divisor = 2, }, > + { .baud = 3000000, .oversampling = 16, .divisor = 1, }, > + { .baud = 3686400, .oversampling = 13, .divisor = 1, }, > + { } /* Terminating Entry */ > +}; > + > +static struct uart_omap_config * > +__serial_omap_get_config(struct uart_port *port, unsigned int baud) > +{ > + struct uart_omap_config *cfg = omap_port_configs; > + > + while (cfg++) { > + if (cfg->baud == baud) > + return cfg; > + } > + > + return NULL; > +} > + > /* > * serial_omap_baud_is_mode16 - check if baud rate is MODE16X > * @port: uart port info > @@ -238,16 +280,12 @@ static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable) > static bool > serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) > { > - unsigned int n13 = port->uartclk / (13 * baud); > - unsigned int n16 = port->uartclk / (16 * baud); > - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > - if(baudAbsDiff13 < 0) > - baudAbsDiff13 = -baudAbsDiff13; > - if(baudAbsDiff16 < 0) > - baudAbsDiff16 = -baudAbsDiff16; > - > - return (baudAbsDiff13 > baudAbsDiff16); > + struct uart_omap_config *cfg = __serial_omap_get_config(port, baud); > + > + if (!cfg) > + return -EINVAL; > + > + return (cfg->oversampling == 16); > } > > /* > @@ -258,13 +296,12 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) > static unsigned int > serial_omap_get_divisor(struct uart_port *port, unsigned int baud) > { > - unsigned int divisor; > + struct uart_omap_config *cfg = __serial_omap_get_config(port, baud); > > - if (!serial_omap_baud_is_mode16(port, baud)) > - divisor = 13; > - else > - divisor = 16; > - return port->uartclk/(baud * divisor); > + if (!cfg) > + return -EINVAL; > + > + return cfg->divisor; > } > > static void serial_omap_enable_ms(struct uart_port *port) > > -- > balbi ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 6:22 ` Alexey Pelykh @ 2013-09-11 18:38 ` Felipe Balbi 2013-09-11 18:48 ` Alexey Pelykh 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2013-09-11 18:38 UTC (permalink / raw) To: Alexey Pelykh Cc: balbi, Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1745 bytes --] Hi, On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: > Hi Felipe, > > Thanks for finding this issue. Indeed, there is a bug on 3M+ baud > rates. First patch is close to a complete fix, but still contains > div-by-zero issue. Here is my version: > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 816d1a2..808a880 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, > unsigned int baud) > { > unsigned int n13 = port->uartclk / (13 * baud); > unsigned int n16 = port->uartclk / (16 * baud); > - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; > + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; IOW: int baudAbsDiff13 = 0; if (n13) baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); which is exactly what my patch did. I fail to see where division by zero would be coming from. > if(baudAbsDiff13 < 0) > baudAbsDiff13 = -baudAbsDiff13; > if(baudAbsDiff16 < 0) > > > With 48MHz UART clock, it will give > 300: divisor = 12307 (13), real rate 300 (0.000000%) > 600: divisor = 6153 (13), real rate 600 (0.000000%) > 1200: divisor = 3076 (13), real rate 1200 (0.000000%) > 2400: divisor = 1538 (13), real rate 2400 (0.000000%) TRM has these all set with oversampling of 16. In fact only 460800, 921600, 1843200 and 3686400 should be using oversampling of 13. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 18:38 ` Felipe Balbi @ 2013-09-11 18:48 ` Alexey Pelykh 2013-09-11 19:00 ` Felipe Balbi 0 siblings, 1 reply; 12+ messages in thread From: Alexey Pelykh @ 2013-09-11 18:48 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: >> Hi Felipe, >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud >> rates. First patch is close to a complete fix, but still contains >> div-by-zero issue. Here is my version: >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >> index 816d1a2..808a880 100644 >> --- a/drivers/tty/serial/omap-serial.c >> +++ b/drivers/tty/serial/omap-serial.c >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, >> unsigned int baud) >> { >> unsigned int n13 = port->uartclk / (13 * baud); >> unsigned int n16 = port->uartclk / (16 * baud); >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; > > IOW: > > int baudAbsDiff13 = 0; > > if (n13) > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero exception will still occur on 3686400. > > which is exactly what my patch did. I fail to see where division by zero > would be coming from. > >> if(baudAbsDiff13 < 0) >> baudAbsDiff13 = -baudAbsDiff13; >> if(baudAbsDiff16 < 0) >> >> >> With 48MHz UART clock, it will give >> 300: divisor = 12307 (13), real rate 300 (0.000000%) >> 600: divisor = 6153 (13), real rate 600 (0.000000%) >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) > > TRM has these all set with oversampling of 16. In fact only 460800, > 921600, 1843200 and 3686400 should be using oversampling of 13. > That's true, but TRM anyways does not contain all possible baud rates (1M e.g.). IMO, as long as error rate is the same as in TRM, it makes no difference what combination of (mode, divisor) to use. > -- > balbi A complex solution may be implemented: use LUT for baud rates that TRM defines explicitly, and use calculation if lookup failed. Thanks, Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 18:48 ` Alexey Pelykh @ 2013-09-11 19:00 ` Felipe Balbi 2013-09-11 19:19 ` Alexey Pelykh 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2013-09-11 19:00 UTC (permalink / raw) To: Alexey Pelykh Cc: balbi, Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 3142 bytes --] On Wed, Sep 11, 2013 at 09:48:13PM +0300, Alexey Pelykh wrote: > On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: > >> Hi Felipe, > >> > >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud > >> rates. First patch is close to a complete fix, but still contains > >> div-by-zero issue. Here is my version: > >> > >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > >> index 816d1a2..808a880 100644 > >> --- a/drivers/tty/serial/omap-serial.c > >> +++ b/drivers/tty/serial/omap-serial.c > >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, > >> unsigned int baud) > >> { > >> unsigned int n13 = port->uartclk / (13 * baud); > >> unsigned int n16 = port->uartclk / (16 * baud); > >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; > >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; > > > > IOW: > > > > int baudAbsDiff13 = 0; > > > > if (n13) > > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); > > Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero > exception will still occur on 3686400. why, there's no division after that point, right ? Besides, serial_omap_baud_is_mode16() is supposed to return a boolean value. Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using that value with a boolean expression, no divisions whatsoever. Division is done after using serial_omap_baud_is_mode16() to initialize divisor to 13 or 16 (which is a misnamer, since that's the oversampling parameter). > > which is exactly what my patch did. I fail to see where division by zero > > would be coming from. > > > >> if(baudAbsDiff13 < 0) > >> baudAbsDiff13 = -baudAbsDiff13; > >> if(baudAbsDiff16 < 0) > >> > >> > >> With 48MHz UART clock, it will give > >> 300: divisor = 12307 (13), real rate 300 (0.000000%) > >> 600: divisor = 6153 (13), real rate 600 (0.000000%) > >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) > >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) > > > > TRM has these all set with oversampling of 16. In fact only 460800, > > 921600, 1843200 and 3686400 should be using oversampling of 13. > > > > That's true, but TRM anyways does not contain all possible baud rates > (1M e.g.). IMO, as long as error rate is the same as in TRM, > it makes no difference what combination of (mode, divisor) to use. > > > -- > > balbi > > A complex solution may be implemented: use LUT for baud rates that TRM > defines explicitly, and use calculation if lookup failed. why would you try calculating anything if there is nothing in the table which can support it ? Whatever is in the lookup table, are the only baud rates the SoC supports, right ? cheers -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 19:00 ` Felipe Balbi @ 2013-09-11 19:19 ` Alexey Pelykh 2013-09-11 20:47 ` Felipe Balbi 0 siblings, 1 reply; 12+ messages in thread From: Alexey Pelykh @ 2013-09-11 19:19 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List On Wed, Sep 11, 2013 at 10:00 PM, Felipe Balbi <balbi@ti.com> wrote: > On Wed, Sep 11, 2013 at 09:48:13PM +0300, Alexey Pelykh wrote: >> On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: >> > Hi, >> > >> > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: >> >> Hi Felipe, >> >> >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud >> >> rates. First patch is close to a complete fix, but still contains >> >> div-by-zero issue. Here is my version: >> >> >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >> >> index 816d1a2..808a880 100644 >> >> --- a/drivers/tty/serial/omap-serial.c >> >> +++ b/drivers/tty/serial/omap-serial.c >> >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, >> >> unsigned int baud) >> >> { >> >> unsigned int n13 = port->uartclk / (13 * baud); >> >> unsigned int n16 = port->uartclk / (16 * baud); >> >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); >> >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); >> >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; >> >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; >> > >> > IOW: >> > >> > int baudAbsDiff13 = 0; >> > >> > if (n13) >> > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); >> >> Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero >> exception will still occur on 3686400. > > why, there's no division after that point, right ? Besides, > serial_omap_baud_is_mode16() is supposed to return a boolean value. > > Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using > that value with a boolean expression, no divisions whatsoever. Division > is done after using serial_omap_baud_is_mode16() to initialize divisor > to 13 or 16 (which is a misnamer, since that's the oversampling > parameter). > Yes, variables are a bit misnamed, that should be fixed someday. Regarding 0 vs INT_MAX, in case of 0 values will be 300: divisor = 12307 (13) 600: divisor = 6153 (13) 1200: divisor = 3076 (13) 2400: divisor = 1538 (13) 4800: divisor = 625 (16) 9600: divisor = 384 (13) 14400: divisor = 256 (13) 19200: divisor = 192 (13) 28800: divisor = 128 (13) 38400: divisor = 96 (13) 57600: divisor = 64 (13) 115200: divisor = 32 (13) 230400: divisor = 16 (13) 460800: divisor = 8 (13) 921600: divisor = 4 (13) 1000000: divisor = 3 (16) 1843200: divisor = 2 (13) 3000000: divisor = 1 (16) 3686400: divisor = 0 (16) << error here, should be 1 (13), as it is with INT_MAX >> > which is exactly what my patch did. I fail to see where division by zero >> > would be coming from. >> > >> >> if(baudAbsDiff13 < 0) >> >> baudAbsDiff13 = -baudAbsDiff13; >> >> if(baudAbsDiff16 < 0) >> >> >> >> >> >> With 48MHz UART clock, it will give >> >> 300: divisor = 12307 (13), real rate 300 (0.000000%) >> >> 600: divisor = 6153 (13), real rate 600 (0.000000%) >> >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) >> >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) >> > >> > TRM has these all set with oversampling of 16. In fact only 460800, >> > 921600, 1843200 and 3686400 should be using oversampling of 13. >> > >> >> That's true, but TRM anyways does not contain all possible baud rates >> (1M e.g.). IMO, as long as error rate is the same as in TRM, >> it makes no difference what combination of (mode, divisor) to use. >> >> > -- >> > balbi >> >> A complex solution may be implemented: use LUT for baud rates that TRM >> defines explicitly, and use calculation if lookup failed. > > why would you try calculating anything if there is nothing in the table > which can support it ? Whatever is in the lookup table, are the only > baud rates the SoC supports, right ? > Actually, I haven't found any statement in TRM, which would mention that listed baudrates in referenced table are the only supported baud rates, and all others are illegal. At least 1M which I use extensively works perfectly, and I can not figure out any idea why it would not do so. General idea behind original commit is "to provide actual baudrate that is as close as possible to requested one", while keeping values in allowed range of divisor and mode. > cheers > > -- > balbi Best regards, Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 19:19 ` Alexey Pelykh @ 2013-09-11 20:47 ` Felipe Balbi 2013-09-12 4:32 ` Alexey Pelykh 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2013-09-11 20:47 UTC (permalink / raw) To: Alexey Pelykh Cc: balbi, Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 5042 bytes --] Hi, On Wed, Sep 11, 2013 at 10:19:47PM +0300, Alexey Pelykh wrote: > On Wed, Sep 11, 2013 at 10:00 PM, Felipe Balbi <balbi@ti.com> wrote: > > On Wed, Sep 11, 2013 at 09:48:13PM +0300, Alexey Pelykh wrote: > >> On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: > >> > Hi, > >> > > >> > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: > >> >> Hi Felipe, > >> >> > >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud > >> >> rates. First patch is close to a complete fix, but still contains > >> >> div-by-zero issue. Here is my version: > >> >> > >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > >> >> index 816d1a2..808a880 100644 > >> >> --- a/drivers/tty/serial/omap-serial.c > >> >> +++ b/drivers/tty/serial/omap-serial.c > >> >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, > >> >> unsigned int baud) > >> >> { > >> >> unsigned int n13 = port->uartclk / (13 * baud); > >> >> unsigned int n16 = port->uartclk / (16 * baud); > >> >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > >> >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > >> >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; > >> >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; > >> > > >> > IOW: > >> > > >> > int baudAbsDiff13 = 0; > >> > > >> > if (n13) > >> > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); > >> > >> Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero > >> exception will still occur on 3686400. > > > > why, there's no division after that point, right ? Besides, > > serial_omap_baud_is_mode16() is supposed to return a boolean value. > > > > Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using > > that value with a boolean expression, no divisions whatsoever. Division > > is done after using serial_omap_baud_is_mode16() to initialize divisor > > to 13 or 16 (which is a misnamer, since that's the oversampling > > parameter). > > > > Yes, variables are a bit misnamed, that should be fixed someday. > Regarding 0 vs INT_MAX, in case of 0 values will be > 300: divisor = 12307 (13) > 600: divisor = 6153 (13) > 1200: divisor = 3076 (13) > 2400: divisor = 1538 (13) > 4800: divisor = 625 (16) > 9600: divisor = 384 (13) > 14400: divisor = 256 (13) > 19200: divisor = 192 (13) > 28800: divisor = 128 (13) > 38400: divisor = 96 (13) > 57600: divisor = 64 (13) > 115200: divisor = 32 (13) > 230400: divisor = 16 (13) > 460800: divisor = 8 (13) > 921600: divisor = 4 (13) > 1000000: divisor = 3 (16) > 1843200: divisor = 2 (13) > 3000000: divisor = 1 (16) > 3686400: divisor = 0 (16) << error here, should be 1 (13), as it is with INT_MAX I get it now... your boolean check wants to use the closer baud to requested baud, so it's mode16 if the delta between baudAbsDiff16 and requested rate is less than delta between baudAbsDiff13 and requested baud. > >> > which is exactly what my patch did. I fail to see where division by zero > >> > would be coming from. > >> > > >> >> if(baudAbsDiff13 < 0) > >> >> baudAbsDiff13 = -baudAbsDiff13; > >> >> if(baudAbsDiff16 < 0) > >> >> > >> >> > >> >> With 48MHz UART clock, it will give > >> >> 300: divisor = 12307 (13), real rate 300 (0.000000%) > >> >> 600: divisor = 6153 (13), real rate 600 (0.000000%) > >> >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) > >> >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) > >> > > >> > TRM has these all set with oversampling of 16. In fact only 460800, > >> > 921600, 1843200 and 3686400 should be using oversampling of 13. > >> > > >> > >> That's true, but TRM anyways does not contain all possible baud rates > >> (1M e.g.). IMO, as long as error rate is the same as in TRM, > >> it makes no difference what combination of (mode, divisor) to use. > >> > >> > -- > >> > balbi > >> > >> A complex solution may be implemented: use LUT for baud rates that TRM > >> defines explicitly, and use calculation if lookup failed. > > > > why would you try calculating anything if there is nothing in the table > > which can support it ? Whatever is in the lookup table, are the only > > baud rates the SoC supports, right ? > > > > Actually, I haven't found any statement in TRM, which would mention > that listed baudrates in referenced table are the only supported baud > rates, > and all others are illegal. "The UART clocks are connected to produce a baud rate of up to 3.6 Mbps. Table 24-122 lists the *supported* baud rates, requested divisor, and corresponding error versus the standard baud rate." > At least 1M which I use extensively works perfectly, and I can not > figure out any idea why it would not do so. it might very well work, but it's not officially *supported* by the IP. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-11 20:47 ` Felipe Balbi @ 2013-09-12 4:32 ` Alexey Pelykh 2013-09-12 4:37 ` Alexey Pelykh 2013-09-12 12:17 ` Felipe Balbi 0 siblings, 2 replies; 12+ messages in thread From: Alexey Pelykh @ 2013-09-12 4:32 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List On Wed, Sep 11, 2013 at 11:47 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Sep 11, 2013 at 10:19:47PM +0300, Alexey Pelykh wrote: >> On Wed, Sep 11, 2013 at 10:00 PM, Felipe Balbi <balbi@ti.com> wrote: >> > On Wed, Sep 11, 2013 at 09:48:13PM +0300, Alexey Pelykh wrote: >> >> On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: >> >> > Hi, >> >> > >> >> > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: >> >> >> Hi Felipe, >> >> >> >> >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud >> >> >> rates. First patch is close to a complete fix, but still contains >> >> >> div-by-zero issue. Here is my version: >> >> >> >> >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >> >> >> index 816d1a2..808a880 100644 >> >> >> --- a/drivers/tty/serial/omap-serial.c >> >> >> +++ b/drivers/tty/serial/omap-serial.c >> >> >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, >> >> >> unsigned int baud) >> >> >> { >> >> >> unsigned int n13 = port->uartclk / (13 * baud); >> >> >> unsigned int n16 = port->uartclk / (16 * baud); >> >> >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); >> >> >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); >> >> >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; >> >> >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; >> >> > >> >> > IOW: >> >> > >> >> > int baudAbsDiff13 = 0; >> >> > >> >> > if (n13) >> >> > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); >> >> >> >> Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero >> >> exception will still occur on 3686400. >> > >> > why, there's no division after that point, right ? Besides, >> > serial_omap_baud_is_mode16() is supposed to return a boolean value. >> > >> > Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using >> > that value with a boolean expression, no divisions whatsoever. Division >> > is done after using serial_omap_baud_is_mode16() to initialize divisor >> > to 13 or 16 (which is a misnamer, since that's the oversampling >> > parameter). >> > >> >> Yes, variables are a bit misnamed, that should be fixed someday. >> Regarding 0 vs INT_MAX, in case of 0 values will be >> 300: divisor = 12307 (13) >> 600: divisor = 6153 (13) >> 1200: divisor = 3076 (13) >> 2400: divisor = 1538 (13) >> 4800: divisor = 625 (16) >> 9600: divisor = 384 (13) >> 14400: divisor = 256 (13) >> 19200: divisor = 192 (13) >> 28800: divisor = 128 (13) >> 38400: divisor = 96 (13) >> 57600: divisor = 64 (13) >> 115200: divisor = 32 (13) >> 230400: divisor = 16 (13) >> 460800: divisor = 8 (13) >> 921600: divisor = 4 (13) >> 1000000: divisor = 3 (16) >> 1843200: divisor = 2 (13) >> 3000000: divisor = 1 (16) >> 3686400: divisor = 0 (16) << error here, should be 1 (13), as it is with INT_MAX > > I get it now... your boolean check wants to use the closer baud to > requested baud, so it's mode16 if the delta between baudAbsDiff16 and > requested rate is less than delta between baudAbsDiff13 and requested > baud. > >> >> > which is exactly what my patch did. I fail to see where division by zero >> >> > would be coming from. >> >> > >> >> >> if(baudAbsDiff13 < 0) >> >> >> baudAbsDiff13 = -baudAbsDiff13; >> >> >> if(baudAbsDiff16 < 0) >> >> >> >> >> >> >> >> >> With 48MHz UART clock, it will give >> >> >> 300: divisor = 12307 (13), real rate 300 (0.000000%) >> >> >> 600: divisor = 6153 (13), real rate 600 (0.000000%) >> >> >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) >> >> >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) >> >> > >> >> > TRM has these all set with oversampling of 16. In fact only 460800, >> >> > 921600, 1843200 and 3686400 should be using oversampling of 13. >> >> > >> >> >> >> That's true, but TRM anyways does not contain all possible baud rates >> >> (1M e.g.). IMO, as long as error rate is the same as in TRM, >> >> it makes no difference what combination of (mode, divisor) to use. >> >> >> >> > -- >> >> > balbi >> >> >> >> A complex solution may be implemented: use LUT for baud rates that TRM >> >> defines explicitly, and use calculation if lookup failed. >> > >> > why would you try calculating anything if there is nothing in the table >> > which can support it ? Whatever is in the lookup table, are the only >> > baud rates the SoC supports, right ? >> > >> >> Actually, I haven't found any statement in TRM, which would mention >> that listed baudrates in referenced table are the only supported baud >> rates, >> and all others are illegal. > > "The UART clocks are connected to produce a baud rate of up to 3.6 Mbps. > Table 24-122 lists the *supported* baud rates, requested divisor, and > corresponding error versus the standard baud rate." > >> At least 1M which I use extensively works perfectly, and I can not >> figure out any idea why it would not do so. > > it might very well work, but it's not officially *supported* by the IP. That's true, but I don't see any reason why driver should disallow usage of baud rates that are not supported, but possible by hardware: "The UART clocks are connected to produce a baud rate of up to 3.6M bits/s." > > -- > balbi I've changed calculation a bit to give priority to mode16, and now it gives TRM table as-is + extra baud rates 300: divisor = 10000 (16), real rate 300 (0.000000%) 600: divisor = 5000 (16), real rate 600 (0.000000%) 1200: divisor = 2500 (16), real rate 1200 (0.000000%) 2400: divisor = 1250 (16), real rate 2400 (0.000000%) 4800: divisor = 625 (16), real rate 4800 (0.000000%) 9600: divisor = 312 (16), real rate 9615 (0.156250%) 14400: divisor = 208 (16), real rate 14423 (0.159722%) 19200: divisor = 156 (16), real rate 19230 (0.156250%) 28800: divisor = 104 (16), real rate 28846 (0.159722%) 38400: divisor = 78 (16), real rate 38461 (0.158854%) 57600: divisor = 52 (16), real rate 57692 (0.159722%) 115200: divisor = 26 (16), real rate 115384 (0.159722%) 230400: divisor = 13 (16), real rate 230769 (0.160156%) 460800: divisor = 8 (13), real rate 461538 (0.160156%) 921600: divisor = 4 (13), real rate 923076 (0.160156%) 1000000: divisor = 3 (16), real rate 1000000 (0.000000%) 1843200: divisor = 2 (13), real rate 1846153 (0.160211%) 3000000: divisor = 1 (16), real rate 3000000 (0.000000%) 3686400: divisor = 1 (13), real rate 3692307 (0.160238%) If that's acceptable behavior, I'll prepare a patch. Thanks, Alexey ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-12 4:32 ` Alexey Pelykh @ 2013-09-12 4:37 ` Alexey Pelykh 2013-09-12 12:21 ` Felipe Balbi 2013-09-12 12:17 ` Felipe Balbi 1 sibling, 1 reply; 12+ messages in thread From: Alexey Pelykh @ 2013-09-12 4:37 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List Actually, here it is, but not formatted properly diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index 816d1a2..146e712 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -240,14 +240,14 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) { unsigned int n13 = port->uartclk / (13 * baud); unsigned int n16 = port->uartclk / (16 * baud); - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; if(baudAbsDiff13 < 0) baudAbsDiff13 = -baudAbsDiff13; if(baudAbsDiff16 < 0) baudAbsDiff16 = -baudAbsDiff16; - return (baudAbsDiff13 > baudAbsDiff16); + return (baudAbsDiff13 >= baudAbsDiff16); } /* @@ -258,13 +258,13 @@ serial_omap_baud_is_mode16(struct uart_port *port, unsigned int baud) static unsigned int serial_omap_get_divisor(struct uart_port *port, unsigned int baud) { - unsigned int divisor; + unsigned int mode; if (!serial_omap_baud_is_mode16(port, baud)) - divisor = 13; + mode = 13; else - divisor = 16; - return port->uartclk/(baud * divisor); + mode = 16; + return port->uartclk/(mode * baud); } static void serial_omap_enable_ms(struct uart_port *port) On Thu, Sep 12, 2013 at 7:32 AM, Alexey Pelykh <alexey.pelykh@gmail.com> wrote: > On Wed, Sep 11, 2013 at 11:47 PM, Felipe Balbi <balbi@ti.com> wrote: >> Hi, >> >> On Wed, Sep 11, 2013 at 10:19:47PM +0300, Alexey Pelykh wrote: >>> On Wed, Sep 11, 2013 at 10:00 PM, Felipe Balbi <balbi@ti.com> wrote: >>> > On Wed, Sep 11, 2013 at 09:48:13PM +0300, Alexey Pelykh wrote: >>> >> On Wed, Sep 11, 2013 at 9:38 PM, Felipe Balbi <balbi@ti.com> wrote: >>> >> > Hi, >>> >> > >>> >> > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: >>> >> >> Hi Felipe, >>> >> >> >>> >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud >>> >> >> rates. First patch is close to a complete fix, but still contains >>> >> >> div-by-zero issue. Here is my version: >>> >> >> >>> >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >>> >> >> index 816d1a2..808a880 100644 >>> >> >> --- a/drivers/tty/serial/omap-serial.c >>> >> >> +++ b/drivers/tty/serial/omap-serial.c >>> >> >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, >>> >> >> unsigned int baud) >>> >> >> { >>> >> >> unsigned int n13 = port->uartclk / (13 * baud); >>> >> >> unsigned int n16 = port->uartclk / (16 * baud); >>> >> >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); >>> >> >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); >>> >> >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; >>> >> >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; >>> >> > >>> >> > IOW: >>> >> > >>> >> > int baudAbsDiff13 = 0; >>> >> > >>> >> > if (n13) >>> >> > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); >>> >> >>> >> Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero >>> >> exception will still occur on 3686400. >>> > >>> > why, there's no division after that point, right ? Besides, >>> > serial_omap_baud_is_mode16() is supposed to return a boolean value. >>> > >>> > Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using >>> > that value with a boolean expression, no divisions whatsoever. Division >>> > is done after using serial_omap_baud_is_mode16() to initialize divisor >>> > to 13 or 16 (which is a misnamer, since that's the oversampling >>> > parameter). >>> > >>> >>> Yes, variables are a bit misnamed, that should be fixed someday. >>> Regarding 0 vs INT_MAX, in case of 0 values will be >>> 300: divisor = 12307 (13) >>> 600: divisor = 6153 (13) >>> 1200: divisor = 3076 (13) >>> 2400: divisor = 1538 (13) >>> 4800: divisor = 625 (16) >>> 9600: divisor = 384 (13) >>> 14400: divisor = 256 (13) >>> 19200: divisor = 192 (13) >>> 28800: divisor = 128 (13) >>> 38400: divisor = 96 (13) >>> 57600: divisor = 64 (13) >>> 115200: divisor = 32 (13) >>> 230400: divisor = 16 (13) >>> 460800: divisor = 8 (13) >>> 921600: divisor = 4 (13) >>> 1000000: divisor = 3 (16) >>> 1843200: divisor = 2 (13) >>> 3000000: divisor = 1 (16) >>> 3686400: divisor = 0 (16) << error here, should be 1 (13), as it is with INT_MAX >> >> I get it now... your boolean check wants to use the closer baud to >> requested baud, so it's mode16 if the delta between baudAbsDiff16 and >> requested rate is less than delta between baudAbsDiff13 and requested >> baud. >> >>> >> > which is exactly what my patch did. I fail to see where division by zero >>> >> > would be coming from. >>> >> > >>> >> >> if(baudAbsDiff13 < 0) >>> >> >> baudAbsDiff13 = -baudAbsDiff13; >>> >> >> if(baudAbsDiff16 < 0) >>> >> >> >>> >> >> >>> >> >> With 48MHz UART clock, it will give >>> >> >> 300: divisor = 12307 (13), real rate 300 (0.000000%) >>> >> >> 600: divisor = 6153 (13), real rate 600 (0.000000%) >>> >> >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) >>> >> >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) >>> >> > >>> >> > TRM has these all set with oversampling of 16. In fact only 460800, >>> >> > 921600, 1843200 and 3686400 should be using oversampling of 13. >>> >> > >>> >> >>> >> That's true, but TRM anyways does not contain all possible baud rates >>> >> (1M e.g.). IMO, as long as error rate is the same as in TRM, >>> >> it makes no difference what combination of (mode, divisor) to use. >>> >> >>> >> > -- >>> >> > balbi >>> >> >>> >> A complex solution may be implemented: use LUT for baud rates that TRM >>> >> defines explicitly, and use calculation if lookup failed. >>> > >>> > why would you try calculating anything if there is nothing in the table >>> > which can support it ? Whatever is in the lookup table, are the only >>> > baud rates the SoC supports, right ? >>> > >>> >>> Actually, I haven't found any statement in TRM, which would mention >>> that listed baudrates in referenced table are the only supported baud >>> rates, >>> and all others are illegal. >> >> "The UART clocks are connected to produce a baud rate of up to 3.6 Mbps. >> Table 24-122 lists the *supported* baud rates, requested divisor, and >> corresponding error versus the standard baud rate." >> >>> At least 1M which I use extensively works perfectly, and I can not >>> figure out any idea why it would not do so. >> >> it might very well work, but it's not officially *supported* by the IP. > > That's true, but I don't see any reason why driver should disallow > usage of baud rates that are not supported, but possible by hardware: > "The UART clocks are connected to produce a baud rate of up to 3.6M bits/s." > >> >> -- >> balbi > > I've changed calculation a bit to give priority to mode16, and now it > gives TRM table as-is + extra baud rates > 300: divisor = 10000 (16), real rate 300 (0.000000%) > 600: divisor = 5000 (16), real rate 600 (0.000000%) > 1200: divisor = 2500 (16), real rate 1200 (0.000000%) > 2400: divisor = 1250 (16), real rate 2400 (0.000000%) > 4800: divisor = 625 (16), real rate 4800 (0.000000%) > 9600: divisor = 312 (16), real rate 9615 (0.156250%) > 14400: divisor = 208 (16), real rate 14423 (0.159722%) > 19200: divisor = 156 (16), real rate 19230 (0.156250%) > 28800: divisor = 104 (16), real rate 28846 (0.159722%) > 38400: divisor = 78 (16), real rate 38461 (0.158854%) > 57600: divisor = 52 (16), real rate 57692 (0.159722%) > 115200: divisor = 26 (16), real rate 115384 (0.159722%) > 230400: divisor = 13 (16), real rate 230769 (0.160156%) > 460800: divisor = 8 (13), real rate 461538 (0.160156%) > 921600: divisor = 4 (13), real rate 923076 (0.160156%) > 1000000: divisor = 3 (16), real rate 1000000 (0.000000%) > 1843200: divisor = 2 (13), real rate 1846153 (0.160211%) > 3000000: divisor = 1 (16), real rate 3000000 (0.000000%) > 3686400: divisor = 1 (13), real rate 3692307 (0.160238%) > > If that's acceptable behavior, I'll prepare a patch. > > Thanks, > Alexey ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-12 4:37 ` Alexey Pelykh @ 2013-09-12 12:21 ` Felipe Balbi 2013-09-12 14:38 ` Alexey Pelykh 0 siblings, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2013-09-12 12:21 UTC (permalink / raw) To: Alexey Pelykh Cc: balbi, Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 1698 bytes --] Hi, On Thu, Sep 12, 2013 at 07:37:07AM +0300, Alexey Pelykh wrote: > Actually, here it is, but not formatted properly > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index 816d1a2..146e712 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -240,14 +240,14 @@ serial_omap_baud_is_mode16(struct uart_port > *port, unsigned int baud) > { > unsigned int n13 = port->uartclk / (13 * baud); > unsigned int n16 = port->uartclk / (16 * baud); > - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; > + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; > if(baudAbsDiff13 < 0) > baudAbsDiff13 = -baudAbsDiff13; > if(baudAbsDiff16 < 0) > baudAbsDiff16 = -baudAbsDiff16; > > - return (baudAbsDiff13 > baudAbsDiff16); > + return (baudAbsDiff13 >= baudAbsDiff16); > } > > /* > @@ -258,13 +258,13 @@ serial_omap_baud_is_mode16(struct uart_port > *port, unsigned int baud) > static unsigned int > serial_omap_get_divisor(struct uart_port *port, unsigned int baud) > { > - unsigned int divisor; > + unsigned int mode; > > if (!serial_omap_baud_is_mode16(port, baud)) > - divisor = 13; > + mode = 13; > else > - divisor = 16; > - return port->uartclk/(baud * divisor); > + mode = 16; > + return port->uartclk/(mode * baud); > } > > static void serial_omap_enable_ms(struct uart_port *port) looks good to me, but I'd split it as suggested as a reply to previous email. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-12 12:21 ` Felipe Balbi @ 2013-09-12 14:38 ` Alexey Pelykh 0 siblings, 0 replies; 12+ messages in thread From: Alexey Pelykh @ 2013-09-12 14:38 UTC (permalink / raw) To: balbi Cc: Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List On Thu, Sep 12, 2013 at 3:21 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Thu, Sep 12, 2013 at 07:37:07AM +0300, Alexey Pelykh wrote: >> Actually, here it is, but not formatted properly >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c >> index 816d1a2..146e712 100644 >> --- a/drivers/tty/serial/omap-serial.c >> +++ b/drivers/tty/serial/omap-serial.c >> @@ -240,14 +240,14 @@ serial_omap_baud_is_mode16(struct uart_port >> *port, unsigned int baud) >> { >> unsigned int n13 = port->uartclk / (13 * baud); >> unsigned int n16 = port->uartclk / (16 * baud); >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; >> if(baudAbsDiff13 < 0) >> baudAbsDiff13 = -baudAbsDiff13; >> if(baudAbsDiff16 < 0) >> baudAbsDiff16 = -baudAbsDiff16; >> >> - return (baudAbsDiff13 > baudAbsDiff16); >> + return (baudAbsDiff13 >= baudAbsDiff16); >> } >> >> /* >> @@ -258,13 +258,13 @@ serial_omap_baud_is_mode16(struct uart_port >> *port, unsigned int baud) >> static unsigned int >> serial_omap_get_divisor(struct uart_port *port, unsigned int baud) >> { >> - unsigned int divisor; >> + unsigned int mode; >> >> if (!serial_omap_baud_is_mode16(port, baud)) >> - divisor = 13; >> + mode = 13; >> else >> - divisor = 16; >> - return port->uartclk/(baud * divisor); >> + mode = 16; >> + return port->uartclk/(mode * baud); >> } >> >> static void serial_omap_enable_ms(struct uart_port *port) > > looks good to me, but I'd split it as suggested as a reply to previous > email. Well, the fix of baud rate calculation (giving mode-16 priority) is a simple >= instead of >. But indeed, probably commits should be different in terms of their subject. > > -- > balbi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: commit 5fe212364 causes division by zero with large bauds 2013-09-12 4:32 ` Alexey Pelykh 2013-09-12 4:37 ` Alexey Pelykh @ 2013-09-12 12:17 ` Felipe Balbi 1 sibling, 0 replies; 12+ messages in thread From: Felipe Balbi @ 2013-09-12 12:17 UTC (permalink / raw) To: Alexey Pelykh Cc: balbi, Tony Lindgren, Greg KH, Linux OMAP Mailing List, linux-serial, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 6899 bytes --] Hi, On Thu, Sep 12, 2013 at 07:32:54AM +0300, Alexey Pelykh wrote: > >> >> > On Wed, Sep 11, 2013 at 09:22:26AM +0300, Alexey Pelykh wrote: > >> >> >> Hi Felipe, > >> >> >> > >> >> >> Thanks for finding this issue. Indeed, there is a bug on 3M+ baud > >> >> >> rates. First patch is close to a complete fix, but still contains > >> >> >> div-by-zero issue. Here is my version: > >> >> >> > >> >> >> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > >> >> >> index 816d1a2..808a880 100644 > >> >> >> --- a/drivers/tty/serial/omap-serial.c > >> >> >> +++ b/drivers/tty/serial/omap-serial.c > >> >> >> @@ -240,8 +240,8 @@ serial_omap_baud_is_mode16(struct uart_port *port, > >> >> >> unsigned int baud) > >> >> >> { > >> >> >> unsigned int n13 = port->uartclk / (13 * baud); > >> >> >> unsigned int n16 = port->uartclk / (16 * baud); > >> >> >> - int baudAbsDiff13 = baud - (port->uartclk / (13 * n13)); > >> >> >> - int baudAbsDiff16 = baud - (port->uartclk / (16 * n16)); > >> >> >> + int baudAbsDiff13 = n13 ? (baud - (port->uartclk / (13 * n13))) : INT_MAX; > >> >> >> + int baudAbsDiff16 = n16 ? (baud - (port->uartclk / (16 * n16))) : INT_MAX; > >> >> > > >> >> > IOW: > >> >> > > >> >> > int baudAbsDiff13 = 0; > >> >> > > >> >> > if (n13) > >> >> > baudAbsDiff13 = (baud - (port->uartclk / (13 * n13))); > >> >> > >> >> Not quite same code, INT_MAX instead of 0. With 0 a div-by-zero > >> >> exception will still occur on 3686400. > >> > > >> > why, there's no division after that point, right ? Besides, > >> > serial_omap_baud_is_mode16() is supposed to return a boolean value. > >> > > >> > Setting baudAbsDiff1[36] to 0 will cause no problems, you're only using > >> > that value with a boolean expression, no divisions whatsoever. Division > >> > is done after using serial_omap_baud_is_mode16() to initialize divisor > >> > to 13 or 16 (which is a misnamer, since that's the oversampling > >> > parameter). > >> > > >> > >> Yes, variables are a bit misnamed, that should be fixed someday. > >> Regarding 0 vs INT_MAX, in case of 0 values will be > >> 300: divisor = 12307 (13) > >> 600: divisor = 6153 (13) > >> 1200: divisor = 3076 (13) > >> 2400: divisor = 1538 (13) > >> 4800: divisor = 625 (16) > >> 9600: divisor = 384 (13) > >> 14400: divisor = 256 (13) > >> 19200: divisor = 192 (13) > >> 28800: divisor = 128 (13) > >> 38400: divisor = 96 (13) > >> 57600: divisor = 64 (13) > >> 115200: divisor = 32 (13) > >> 230400: divisor = 16 (13) > >> 460800: divisor = 8 (13) > >> 921600: divisor = 4 (13) > >> 1000000: divisor = 3 (16) > >> 1843200: divisor = 2 (13) > >> 3000000: divisor = 1 (16) > >> 3686400: divisor = 0 (16) << error here, should be 1 (13), as it is with INT_MAX > > > > I get it now... your boolean check wants to use the closer baud to > > requested baud, so it's mode16 if the delta between baudAbsDiff16 and > > requested rate is less than delta between baudAbsDiff13 and requested > > baud. > > > >> >> > which is exactly what my patch did. I fail to see where division by zero > >> >> > would be coming from. > >> >> > > >> >> >> if(baudAbsDiff13 < 0) > >> >> >> baudAbsDiff13 = -baudAbsDiff13; > >> >> >> if(baudAbsDiff16 < 0) > >> >> >> > >> >> >> > >> >> >> With 48MHz UART clock, it will give > >> >> >> 300: divisor = 12307 (13), real rate 300 (0.000000%) > >> >> >> 600: divisor = 6153 (13), real rate 600 (0.000000%) > >> >> >> 1200: divisor = 3076 (13), real rate 1200 (0.000000%) > >> >> >> 2400: divisor = 1538 (13), real rate 2400 (0.000000%) > >> >> > > >> >> > TRM has these all set with oversampling of 16. In fact only 460800, > >> >> > 921600, 1843200 and 3686400 should be using oversampling of 13. > >> >> > > >> >> > >> >> That's true, but TRM anyways does not contain all possible baud rates > >> >> (1M e.g.). IMO, as long as error rate is the same as in TRM, > >> >> it makes no difference what combination of (mode, divisor) to use. > >> >> > >> >> > -- > >> >> > balbi > >> >> > >> >> A complex solution may be implemented: use LUT for baud rates that TRM > >> >> defines explicitly, and use calculation if lookup failed. > >> > > >> > why would you try calculating anything if there is nothing in the table > >> > which can support it ? Whatever is in the lookup table, are the only > >> > baud rates the SoC supports, right ? > >> > > >> > >> Actually, I haven't found any statement in TRM, which would mention > >> that listed baudrates in referenced table are the only supported baud > >> rates, > >> and all others are illegal. > > > > "The UART clocks are connected to produce a baud rate of up to 3.6 Mbps. > > Table 24-122 lists the *supported* baud rates, requested divisor, and > > corresponding error versus the standard baud rate." > > > >> At least 1M which I use extensively works perfectly, and I can not > >> figure out any idea why it would not do so. > > > > it might very well work, but it's not officially *supported* by the IP. > > That's true, but I don't see any reason why driver should disallow > usage of baud rates that are not supported, but possible by hardware: > "The UART clocks are connected to produce a baud rate of up to 3.6M bits/s." fair point. > I've changed calculation a bit to give priority to mode16, and now it > gives TRM table as-is + extra baud rates > 300: divisor = 10000 (16), real rate 300 (0.000000%) > 600: divisor = 5000 (16), real rate 600 (0.000000%) > 1200: divisor = 2500 (16), real rate 1200 (0.000000%) > 2400: divisor = 1250 (16), real rate 2400 (0.000000%) > 4800: divisor = 625 (16), real rate 4800 (0.000000%) > 9600: divisor = 312 (16), real rate 9615 (0.156250%) > 14400: divisor = 208 (16), real rate 14423 (0.159722%) > 19200: divisor = 156 (16), real rate 19230 (0.156250%) > 28800: divisor = 104 (16), real rate 28846 (0.159722%) > 38400: divisor = 78 (16), real rate 38461 (0.158854%) > 57600: divisor = 52 (16), real rate 57692 (0.159722%) > 115200: divisor = 26 (16), real rate 115384 (0.159722%) > 230400: divisor = 13 (16), real rate 230769 (0.160156%) > 460800: divisor = 8 (13), real rate 461538 (0.160156%) > 921600: divisor = 4 (13), real rate 923076 (0.160156%) > 1000000: divisor = 3 (16), real rate 1000000 (0.000000%) > 1843200: divisor = 2 (13), real rate 1846153 (0.160211%) > 3000000: divisor = 1 (16), real rate 3000000 (0.000000%) > 3686400: divisor = 1 (13), real rate 3692307 (0.160238%) > > If that's acceptable behavior, I'll prepare a patch. Sure but I'd suggest splitting the patch so we can get the bugfix in mainline during this -rc. So first patch would only fix the division-by-zero bug and second patch would 'fix' baudrate calculation. Sounds good ? -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-09-12 14:38 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-10 19:09 commit 5fe212364 causes division by zero with large bauds Felipe Balbi 2013-09-11 6:22 ` Alexey Pelykh 2013-09-11 18:38 ` Felipe Balbi 2013-09-11 18:48 ` Alexey Pelykh 2013-09-11 19:00 ` Felipe Balbi 2013-09-11 19:19 ` Alexey Pelykh 2013-09-11 20:47 ` Felipe Balbi 2013-09-12 4:32 ` Alexey Pelykh 2013-09-12 4:37 ` Alexey Pelykh 2013-09-12 12:21 ` Felipe Balbi 2013-09-12 14:38 ` Alexey Pelykh 2013-09-12 12:17 ` Felipe Balbi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).