Re: [PATCH] hw/char/imx_serial: Implement receive FIFO and ageing timer for imx serial.

[Peter Maydell]

On Thu, 11 Jan 2024 at 13:33, Rayhan Faizel <rayhan.faizel@gmail.com> wrote:
>
> This patch implements a 32 half word FIFO as per imx serial device
> specifications. If a non empty FIFO is below the trigger level, an ageing
> timer will tick for a duration of 8 characters. On expiry, AGTIM will be set
> triggering an interrupt. AGTIM timer resets when there is activity in
> the receive FIFO.
>
> Otherwise, RRDY is set when trigger level is
> exceeded. The receive trigger level is 8 in newer kernel versions and 1 in
> older ones.
>
> Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>

Hi; thanks for this patch. My main question is whether
we can use our generic FIFO implementation (include/qemu/fifo32.h)
rather than hand-rolling one. Otherwise the remarks below
are all minor things.

> ---
>  hw/char/imx_serial.c         | 116 ++++++++++++++++++++++++++++++-----
>  include/hw/char/imx_serial.h |  22 ++++++-
>  2 files changed, 121 insertions(+), 17 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index 1df862eb7f..6ec67be282 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -44,7 +44,11 @@ static const VMStateDescription vmstate_imx_serial = {
>      .version_id = 2,
>      .minimum_version_id = 2,
>      .fields = (const VMStateField[]) {
> -        VMSTATE_INT32(readbuff, IMXSerialState),
> +        VMSTATE_INT32_ARRAY(rx_fifo, IMXSerialState,
> +                            FIFO_SIZE),
> +        VMSTATE_UINT8(rx_start, IMXSerialState),
> +        VMSTATE_UINT8(rx_end, IMXSerialState),
> +        VMSTATE_UINT8(rx_used, IMXSerialState),

If we change the vmstate fields, we need to also increment
the version_id and minimum_version_id, because this is a
migration compatibility break. The compat break should also
be noted in the commit message.

(There are ways to maintain compatibility, but for the imx6
boards we don't really care about cross-version compat, so
bumping the version numbers lets us at least give the user
a comprehensible error message if they try something that
we don't support.)

>          VMSTATE_UINT32(usr1, IMXSerialState),
>          VMSTATE_UINT32(usr2, IMXSerialState),
>          VMSTATE_UINT32(ucr1, IMXSerialState),
> @@ -64,13 +68,16 @@ static void imx_update(IMXSerialState *s)
>      uint32_t usr1;
>      uint32_t usr2;
>      uint32_t mask;
> -
>      /*

Stray whitespace change.

>       * Lucky for us TRDY and RRDY has the same offset in both USR1 and
>       * UCR1, so we can get away with something as simple as the
>       * following:
>       */
>      usr1 = s->usr1 & s->ucr1 & (USR1_TRDY | USR1_RRDY);
> +    /*
> +     * Interrupt if AGTIM is set (ageing timer interrupt in RxFIFO)
> +     */
> +    usr1 |= (s->ucr2 & UCR2_ATEN) ? (s->usr1 & USR1_AGTIM) : 0;
>      /*
>       * Bits that we want in USR2 are not as conveniently laid out,
>       * unfortunately.
> @@ -85,11 +92,73 @@ static void imx_update(IMXSerialState *s)
>      usr2 = s->usr2 & mask;
>
>      qemu_set_irq(s->irq, usr1 || usr2);
> +

Another stray whitespace change.

>  }
>
> -static void imx_serial_reset(IMXSerialState *s)
> +static void imx_serial_rx_fifo_push(IMXSerialState *s, uint32_t value)
> +{
> +    uint8_t new_rx_end = (s->rx_end + 1) % FIFO_SIZE;
> +    s->rx_used++;
> +
> +    if (s->rx_used > FIFO_SIZE) {
> +        /*
> +         * Handle 33rd character in filled RxFIFO
> +         */
> +        s->rx_start = (s->rx_start + 1) % FIFO_SIZE;
> +        s->rx_used--;
> +    }
> +    s->rx_fifo[s->rx_end] = value;
> +    s->rx_end = new_rx_end;
> +}

Is there a reason we can't use our generic 32-bit
FIFO implementation (include/qemu/fifo32.h) ?

> +
> +static int32_t imx_serial_rx_fifo_pop(IMXSerialState *s)
> +{
> +    int32_t front;
> +    if (s->rx_used == 0) {
> +        /*
> +         * FIFO is already empty
> +         */
> +        return URXD_ERR;
> +    }
> +    front = s->rx_fifo[s->rx_start];
> +
> +    s->rx_start = (s->rx_start + 1) % FIFO_SIZE;
> +    s->rx_used--;
> +
> +    return front;
> +}
> +
> +static void imx_serial_rx_fifo_ageing_timer_int(void *opaque)
> +{
> +    IMXSerialState* s = (IMXSerialState *) opaque;

You don't want a whitespace after the cast and before
'opaque', here and below. (I don't know if this is something
scripts/checkpatch.pl will warn about or not.)

> +    s->usr1 |= USR1_AGTIM;
> +
> +    imx_update(s);
> +}
> +
> +static void imx_serial_rx_fifo_ageing_timer_restart(void *opaque)
>  {
> +    /*
> +     * Ageing timer starts ticking when
> +     * RX FIFO is non empty and below trigger level.
> +     * Timer is reset if new character is received or
> +     * a FIFO read occurs.
> +     * Timer triggers an interrupt when duration of
> +     * 8 characters has passed ( assuming 115200 baudrate ).
> +     */
> +    IMXSerialState* s = (IMXSerialState *) opaque;
> +    uint8_t rxtl = s->ufcr & TL_MASK;
> +
> +    if (s->rx_used > 0 && s->rx_used < rxtl) {
> +        timer_mod_ns(&s->ageing_timer,
> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + AGE_DURATION_NS);
> +    } else {
> +        timer_del(&s->ageing_timer);
> +    }
> +}
>
> +static void imx_serial_reset(IMXSerialState *s)
> +{
>      s->usr1 = USR1_TRDY | USR1_RXDS;
>      /*
>       * Fake attachment of a terminal: assert RTS.
> @@ -102,13 +171,20 @@ static void imx_serial_reset(IMXSerialState *s)
>      s->ucr3 = 0x700;
>      s->ubmr = 0;
>      s->ubrc = 4;
> -    s->readbuff = URXD_ERR;
> +
> +
> +    memset(s->rx_fifo, 0, sizeof(s->rx_fifo));
> +    s->rx_used = 0;
> +    s->rx_start = 0;
> +    s->rx_end = 0;
> +
> +    timer_init_ns(&s->ageing_timer, QEMU_CLOCK_VIRTUAL,
> +        imx_serial_rx_fifo_ageing_timer_int, s);

We should only initialize the timer once, in the device's
realize method. In this reset function, we only want to
call timer_del(), to ensure it is not running.

>  }
>
>  static void imx_serial_reset_at_boot(DeviceState *dev)
>  {
>      IMXSerialState *s = IMX_SERIAL(dev);
> -
>      imx_serial_reset(s);
>
>      /*

Another stray whitespace change.

> @@ -126,19 +202,24 @@ static uint64_t imx_serial_read(void *opaque, hwaddr 
> offset,
>  {
>      IMXSerialState *s = (IMXSerialState *)opaque;
>      uint32_t c;
> -
> +    uint8_t rxtl = s->ufcr & TL_MASK;
>      DPRINTF("read(offset=0x%" HWADDR_PRIx ")\n", offset);
> -

Whitespace changes again.

>      switch (offset >> 2) {
>      case 0x0: /* URXD */
> -        c = s->readbuff;
> +        c = imx_serial_rx_fifo_pop(s);
>          if (!(s->uts1 & UTS1_RXEMPTY)) {
>              /* Character is valid */
>              c |= URXD_CHARRDY;
> -            s->usr1 &= ~USR1_RRDY;
> -            s->usr2 &= ~USR2_RDR;
> -            s->uts1 |= UTS1_RXEMPTY;
> +            /* Clear RRDY if below threshold */
> +            if (s->rx_used < rxtl) {
> +                s->usr1 &= ~USR1_RRDY;
> +            }
> +            if (s->rx_used == 0) {
> +                s->usr2 &= ~USR2_RDR;
> +                s->uts1 |= UTS1_RXEMPTY;
> +            }
>              imx_update(s);
> +            imx_serial_rx_fifo_ageing_timer_restart(s);
>              qemu_chr_fe_accept_input(&s->chr);
>          }
>          return c;
> @@ -300,19 +381,24 @@ static void imx_serial_write(void *opaque, hwaddr 
> offset,
>  static int imx_can_receive(void *opaque)
>  {
>      IMXSerialState *s = (IMXSerialState *)opaque;
> -    return !(s->usr1 & USR1_RRDY);
> +    return s->ucr1 & UCR1_RRDYEN &&
> +        s->ucr2 & UCR2_RXEN && s->rx_used < FIFO_SIZE;
>  }
>
>  static void imx_put_data(void *opaque, uint32_t value)
>  {
>      IMXSerialState *s = (IMXSerialState *)opaque;
> -
> +    uint8_t rxtl = s->ufcr & TL_MASK;
>      DPRINTF("received char\n");
> +    imx_serial_rx_fifo_push(s, value);
> +    if (s->rx_used >= rxtl) {
> +        s->usr1 |= USR1_RRDY;
> +    }
> +
> +    imx_serial_rx_fifo_ageing_timer_restart(s);
>
> -    s->usr1 |= USR1_RRDY;
>      s->usr2 |= USR2_RDR;
>      s->uts1 &= ~UTS1_RXEMPTY;
> -    s->readbuff = value;
>      if (value & URXD_BRK) {
>          s->usr2 |= USR2_BRCD;
>      }
> diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
> index b823f94519..86a0a102a5 100644
> --- a/include/hw/char/imx_serial.h
> +++ b/include/hw/char/imx_serial.h
> @@ -25,6 +25,8 @@
>  #define TYPE_IMX_SERIAL "imx.serial"
>  OBJECT_DECLARE_SIMPLE_TYPE(IMXSerialState, IMX_SERIAL)
>
> +#define FIFO_SIZE       32
> +
>  #define URXD_CHARRDY    (1<<15)   /* character read is valid */
>  #define URXD_ERR        (1<<14)   /* Character has error */
>  #define URXD_FRMERR     (1<<12)   /* Character has frame error */
> @@ -65,6 +67,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IMXSerialState, IMX_SERIAL)
>  #define UCR1_TXMPTYEN   (1<<6)    /* Tx Empty Interrupt Enable */
>  #define UCR1_UARTEN     (1<<0)    /* UART Enable */
>
> +#define UCR2_ATEN       BIT(3)    /* Ageing Timer Enable */
>  #define UCR2_TXEN       (1<<2)    /* Transmitter enable */
>  #define UCR2_RXEN       (1<<1)    /* Receiver enable */
>  #define UCR2_SRST       (1<<0)    /* Reset complete */
> @@ -78,13 +81,28 @@ OBJECT_DECLARE_SIMPLE_TYPE(IMXSerialState, IMX_SERIAL)
>  #define UTS1_TXFULL     (1<<4)
>  #define UTS1_RXFULL     (1<<3)
>
> +#define TL_MASK         0x3f
> +
> + /* Bit time in nanoseconds assuming maximum baud rate of 115200 */
> +#define BIT_TIME_NS     8681
> +
> +/* Assume 8 bits per character */
> +#define NUM_BITS        8
> +
> +/* Ageing timer triggers after 8 characters */
> +#define AGE_DURATION_NS (8 * NUM_BITS * BIT_TIME_NS)
> +
>  struct IMXSerialState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> -
>      /*< public >*/
>      MemoryRegion iomem;
> -    int32_t readbuff;
> +    QEMUTimer ageing_timer;
> +
> +    int32_t rx_fifo[FIFO_SIZE];
> +    uint8_t rx_start;
> +    uint8_t rx_end;
> +    uint8_t rx_used;
>
>      uint32_t usr1;
>      uint32_t usr2;
> --
> 2.34.1

thanks
-- PMM