[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/3] hw/char: Convert the Ibex UART to use the qdev Clock
From: |
Alistair Francis |
Subject: |
Re: [PATCH v1 1/3] hw/char: Convert the Ibex UART to use the qdev Clock model |
Date: |
Tue, 7 Jul 2020 10:20:11 -0700 |
On Fri, Jul 3, 2020 at 12:42 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Damien
>
> On 6/30/20 10:12 PM, Alistair Francis wrote:
> > Conver the Ibex UART to use the recently added qdev-clock functions.
>
> Yeah! This is our first user \o/
:)
>
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/char/ibex_uart.h | 2 ++
> > hw/char/ibex_uart.c | 19 ++++++++++++++++++-
> > 2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/char/ibex_uart.h b/include/hw/char/ibex_uart.h
> > index 2bec772615..322bfffd8b 100644
> > --- a/include/hw/char/ibex_uart.h
> > +++ b/include/hw/char/ibex_uart.h
> > @@ -101,6 +101,8 @@ typedef struct {
> > uint32_t uart_val;
> > uint32_t uart_timeout_ctrl;
> >
> > + Clock *f_clk;
> > +
> > CharBackend chr;
> > qemu_irq tx_watermark;
> > qemu_irq rx_watermark;
> > diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
> > index 45cd724998..f967e6919a 100644
> > --- a/hw/char/ibex_uart.c
> > +++ b/hw/char/ibex_uart.c
> > @@ -28,6 +28,7 @@
> > #include "qemu/osdep.h"
> > #include "hw/char/ibex_uart.h"
> > #include "hw/irq.h"
> > +#include "hw/qdev-clock.h"
> > #include "hw/qdev-properties.h"
> > #include "migration/vmstate.h"
> > #include "qemu/log.h"
> > @@ -330,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);
>
> UART_CTRL_NCO is defined as:
>
> #define UART_CTRL_NCO (0xFFFF << 16)
>
> Note for later, convert to the clearer registerfields API?
Done, I have added a patch to do this.
>
> > - baud *= 1000;
> > + baud *= clock_get_hz(s->f_clk);
> > baud >>= 20;
> >
> > s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> > @@ -385,6 +386,18 @@ static void ibex_uart_write(void *opaque, hwaddr addr,
> > }
> > }
> >
> > +static void ibex_uart_clk_update(void *opaque)
> > +{
> > + IbexUartState *s = opaque;
> > +
> > + /* recompute uart's speed on clock change */
> > + uint64_t baud = ((s->uart_ctrl & UART_CTRL_NCO) >> 16);
> > + baud *= clock_get_hz(s->f_clk);
> > + baud >>= 20;
>
> Maybe worth to extract:
>
> uint64_t ibex_uart_get_baud(IbexUartState *s)
> {
> uint64_t baud;
>
> baud = ((s->uart_ctrl & UART_CTRL_NCO) >> 16);
> baud *= clock_get_hz(s->f_clk);
> baud >>= 20;
>
> return baud;
> }
Done.
>
> > +
> > + s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
> > +}
> > +
> > static void fifo_trigger_update(void *opaque)
> > {
> > IbexUartState *s = opaque;
> > @@ -444,6 +457,10 @@ static void ibex_uart_init(Object *obj)
> > {
> > IbexUartState *s = IBEX_UART(obj);
> >
> > + s->f_clk = qdev_init_clock_in(DEVICE(obj), "f_clock",
> > + ibex_uart_clk_update, s);
> > + clock_set_hz(s->f_clk, 50000000);
>
> Can you add a definition for this 50 MHz value:
Done.
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Thanks!
Alistair
>
> > +
> > sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_watermark);
> > sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rx_watermark);
> > sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->tx_empty);
> >
>