hw/char/ibex_uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The xor-as-pow warning in clang actually detected a genuine bug.
Fix it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/char/ibex_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 3e0dd9968e..45cd724998 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -331,7 +331,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
if (value & UART_CTRL_NCO) {
uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
baud *= 1000;
- baud /= 2 ^ 20;
+ baud >>= 20;
s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
}
--
2.26.2
On 6/23/20 2:54 PM, Paolo Bonzini wrote: > The xor-as-pow warning in clang actually detected a genuine bug. > Fix it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/char/ibex_uart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c > index 3e0dd9968e..45cd724998 100644 > --- a/hw/char/ibex_uart.c > +++ b/hw/char/ibex_uart.c > @@ -331,7 +331,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr, > if (value & UART_CTRL_NCO) { > uint64_t baud = ((value & UART_CTRL_NCO) >> 16); > baud *= 1000; > - baud /= 2 ^ 20; > + baud >>= 20; Dividing by 1M instead of 22 seems much more logical, indeed :) It's odd that we are scaling up by 1000, down by 1024*1024, then > > s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10; inverting and scaling by 10. I don't know if there is a more succinct way to write the intended transformation. > } But even without reading the hardware manual to see if the resulting scaling is correct, the typo fix is obvious enough to give: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 23/06/20 22:07, Eric Blake wrote: >> >> uint64_t baud = ((value & UART_CTRL_NCO) >> 16); >> baud *= 1000; >> - baud /= 2 ^ 20; >> + baud >>= 20; > > Dividing by 1M instead of 22 seems much more logical, indeed :) Based on the spec, the "* 1000" is the clock, in other words this is a fixed point value relative to the clock: f_baud = NCO * f_clock / 2^20 The example in the spec (https://docs.opentitan.org/hw/ip/uart/doc/) has f_clock = 50 MHz, while here it's only 1 kHz. And the register is only 16 bit, so the above would only allow a baud rate up to 62 (65535 * 1000 / 2^20). Should the clock be a property of the device instead? Thanks, Paolo > It's odd that we are scaling up by 1000, down by 1024*1024, then > >> s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
On 6/24/20 8:33 AM, Paolo Bonzini wrote: > On 23/06/20 22:07, Eric Blake wrote: >>> >>> uint64_t baud = ((value & UART_CTRL_NCO) >> 16); >>> baud *= 1000; >>> - baud /= 2 ^ 20; >>> + baud >>= 20; >> >> Dividing by 1M instead of 22 seems much more logical, indeed :) > > Based on the spec, the "* 1000" is the clock, in other words this is a > fixed point value relative to the clock: > > f_baud = NCO * f_clock / 2^20 > > The example in the spec (https://docs.opentitan.org/hw/ip/uart/doc/) has > f_clock = 50 MHz, while here it's only 1 kHz. And the register is only > 16 bit, so the above would only allow a baud rate up to 62 (65535 * 1000 > / 2^20). > > Should the clock be a property of the device instead? Ideally the device should use qdev_get_clock_in(), (see docs/devel/clocks.rst): static void ibex_uart_init(Object *obj) { ... s->f_clk = qdev_get_clock_in(DEVICE(obj), "f_clock"); ... Then in ibex_uart_write(): uint64_t baud = ((value & UART_CTRL_NCO) >> 16); baud *= clock_get_hz(dev->f_clk)); baud >>= 20; Devices not using the QDEV_CLOCK API use QDEV properties. > > Thanks, > > Paolo > >> It's odd that we are scaling up by 1000, down by 1024*1024, then >> >>> s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10; > >
cc'ing Alistair as listed maintainer for the device... On Tue, 23 Jun 2020 at 21:07, Paolo Bonzini <pbonzini@redhat.com> wrote: > > The xor-as-pow warning in clang actually detected a genuine bug. > Fix it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/char/ibex_uart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c > index 3e0dd9968e..45cd724998 100644 > --- a/hw/char/ibex_uart.c > +++ b/hw/char/ibex_uart.c > @@ -331,7 +331,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr, > if (value & UART_CTRL_NCO) { > uint64_t baud = ((value & UART_CTRL_NCO) >> 16); > baud *= 1000; > - baud /= 2 ^ 20; > + baud >>= 20; > > s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10; > } > -- > 2.26.2
On Tue, Jun 23, 2020 at 1:07 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > The xor-as-pow warning in clang actually detected a genuine bug. > Fix it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/char/ibex_uart.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c > index 3e0dd9968e..45cd724998 100644 > --- a/hw/char/ibex_uart.c > +++ b/hw/char/ibex_uart.c > @@ -331,7 +331,7 @@ static void ibex_uart_write(void *opaque, hwaddr addr, > if (value & UART_CTRL_NCO) { > uint64_t baud = ((value & UART_CTRL_NCO) >> 16); > baud *= 1000; > - baud /= 2 ^ 20; > + baud >>= 20; Whoops, that is clearly wrong. Thanks for catching this. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10; > } > -- > 2.26.2 > >
© 2016 - 2024 Red Hat, Inc.