| Message ID | 20220121150614.18877-1-jakub.luzny@codasip.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: utils/serial: Round UART8250 baud rate divisor to nearest integer | expand |
On 21 Jan 2022, at 15:06, Jakub Luzny <jakub.luzny@codasip.com> wrote: > > Previously, it was rounded down and that gives suboptimal results when > non-standard clock sources or baud rates are used. > > Signed-off-by: Jakub Luzny <jakub.luzny@codasip.com> > --- > lib/utils/serial/uart8250.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > index 1cf6624..ccc7aa0 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -101,7 +101,7 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > uart8250_in_freq = in_freq; > uart8250_baudrate = baudrate; > > - bdiv = uart8250_in_freq / (16 * uart8250_baudrate); > + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); This could do weird things if the numbers are sufficiently large to overflow (unlikely but maybe possible). You can instead use the simpler: (uart8250_in_freq / (8 * uart8250_baudrate) + 1) / 2 which is approximately what FreeBSD does (it shifts rather than multiplies and divides). Jess
Yes, this could happen if the UART peripheral would run at clocks around 4GHz, which is really unlikely. My solution is basically what the Linux kernel does (through the DIV_ROUND_CLOSEST macro). I consider it more readable than the FreeBSD-like solution you propose. But if you disagree, I can send a patch following your proposal. Jakub On Fri, Jan 21, 2022 at 4:21 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 21 Jan 2022, at 15:06, Jakub Luzny <jakub.luzny@codasip.com> wrote: > > > > Previously, it was rounded down and that gives suboptimal results when > > non-standard clock sources or baud rates are used. > > > > Signed-off-by: Jakub Luzny <jakub.luzny@codasip.com> > > --- > > lib/utils/serial/uart8250.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > > index 1cf6624..ccc7aa0 100644 > > --- a/lib/utils/serial/uart8250.c > > +++ b/lib/utils/serial/uart8250.c > > @@ -101,7 +101,7 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > > uart8250_in_freq = in_freq; > > uart8250_baudrate = baudrate; > > > > - bdiv = uart8250_in_freq / (16 * uart8250_baudrate); > > + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > This could do weird things if the numbers are sufficiently large to > overflow (unlikely but maybe possible). You can instead use the simpler: > > (uart8250_in_freq / (8 * uart8250_baudrate) + 1) / 2 > > which is approximately what FreeBSD does (it shifts rather than > multiplies and divides). > > Jess >
On Fri, Jan 21, 2022 at 8:36 PM Jakub Luzny <jakub.luzny@codasip.com> wrote: > > Previously, it was rounded down and that gives suboptimal results when > non-standard clock sources or baud rates are used. > > Signed-off-by: Jakub Luzny <jakub.luzny@codasip.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > --- > lib/utils/serial/uart8250.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c > index 1cf6624..ccc7aa0 100644 > --- a/lib/utils/serial/uart8250.c > +++ b/lib/utils/serial/uart8250.c > @@ -101,7 +101,7 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, > uart8250_in_freq = in_freq; > uart8250_baudrate = baudrate; > > - bdiv = uart8250_in_freq / (16 * uart8250_baudrate); > + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); > > /* Disable all interrupts */ > set_reg(UART_IER_OFFSET, 0x00); > -- > 2.20.1 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
diff --git a/lib/utils/serial/uart8250.c b/lib/utils/serial/uart8250.c index 1cf6624..ccc7aa0 100644 --- a/lib/utils/serial/uart8250.c +++ b/lib/utils/serial/uart8250.c @@ -101,7 +101,7 @@ int uart8250_init(unsigned long base, u32 in_freq, u32 baudrate, u32 reg_shift, uart8250_in_freq = in_freq; uart8250_baudrate = baudrate; - bdiv = uart8250_in_freq / (16 * uart8250_baudrate); + bdiv = (uart8250_in_freq + 8 * uart8250_baudrate) / (16 * uart8250_baudrate); /* Disable all interrupts */ set_reg(UART_IER_OFFSET, 0x00);
Previously, it was rounded down and that gives suboptimal results when non-standard clock sources or baud rates are used. Signed-off-by: Jakub Luzny <jakub.luzny@codasip.com> --- lib/utils/serial/uart8250.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)