Re: [PATCH] hw/char/pl011: Add support for loopback

[Peter Maydell]

On Wed, 7 Feb 2024 at 05:03, Tong Ho <tong.ho@amd.com> wrote:
>
> This patch adds loopback for sent characters as well as
> modem-control signals.
>
> Loopback of send and modem-control is often used for uart
> self tests in real hardware but missing from current pl011
> model, resulting in self-test failures when running in QEMU.
>
> Signed-off-by: Tong Ho <tong.ho@amd.com>
> Signed-off-by: Francisco Iglesias <francisco.iglesias@amd.com>

Hi; thanks for this patch.

> ---
>  hw/char/pl011.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 855cb82d08..3c0e07aa35 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -121,6 +121,51 @@ static void pl011_update(PL011State *s)
>      }
>  }
>
> +static void pl011_put_fifo(void *opaque, uint32_t value);
> +
> +static bool pl011_is_loopback(PL011State *s)
> +{
> +    return !!(s->cr & (1U << 7));
> +}
> +
> +static void pl011_tx_loopback(PL011State *s, uint32_t value)
> +{
> +    if (pl011_is_loopback(s)) {
> +        pl011_put_fifo(s, value);
> +    }
> +}
> +
> +static uint32_t pl011_cr_loopback(PL011State *s, bool update)
> +{
> +    uint32_t cr = s->cr;
> +    uint32_t fr = s->flags;
> +    uint32_t ri = 1 << 8, dcd = 1 << 2, dsr = 1 << 1, cts = 0;
> +    uint32_t out2 = 1 << 13, out1 = 1 << 12, rts = 1 << 11, dtr = 1 << 10;

Please don't use local variables for these -- define some
CR_whatever constants at the top of the file for the CR bits,
as we already do for eg LCR bits.

We already have some PL011_FLAG_* constants for FR bit values;
you can extend those to cover the new bits you want to set here.

> +
> +    if (!pl011_is_loopback(s)) {
> +        return fr;
> +    }
> +
> +    fr &= ~(ri | dcd | dsr | cts);
> +    fr |= (cr & out2) ?  ri : 0;   /* FR.RI  <= CR.Out2 */
> +    fr |= (cr & out1) ? dcd : 0;   /* FR.DCD <= CR.Out1 */
> +    fr |= (cr &  rts) ? cts : 0;   /* FR.CTS <= CR.RTS */
> +    fr |= (cr &  dtr) ? dsr : 0;   /* FR.DSR <= CR.DTR */
> +
> +    if (!update) {
> +        return fr;
> +    }
> +
> +    s->int_level &= ~(INT_DSR | INT_DCD | INT_CTS | INT_RI);
> +    s->int_level |= (fr & dsr) ? INT_DSR : 0;
> +    s->int_level |= (fr & dcd) ? INT_DCD : 0;
> +    s->int_level |= (fr & cts) ? INT_CTS : 0;
> +    s->int_level |= (fr &  ri) ? INT_RI  : 0;
> +    pl011_update(s);
> +
> +    return fr;
> +}

I think we should not call this function "pl011_cr_loopback()",
because it handles all cases, not merely the "loopback enabled"
case. It also seems to be doing double duty as both
"return the flags register value" and "handle a write to
the cr register" -- I think it wolud be clearer to separate
those two out.

> +
>  static bool pl011_is_fifo_enabled(PL011State *s)
>  {
>      return (s->lcr & LCR_FEN) != 0;
> @@ -172,7 +217,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
>          r = s->rsr;
>          break;
>      case 6: /* UARTFR */
> -        r = s->flags;
> +        r = pl011_cr_loopback(s, false);
>          break;
>      case 8: /* UARTILPR */
>          r = s->ilpr;
> @@ -267,6 +312,7 @@ static void pl011_write(void *opaque, hwaddr offset,
>           * qemu_chr_fe_write and background I/O callbacks */
>          qemu_chr_fe_write_all(&s->chr, &ch, 1);
>          s->int_level |= INT_TX;
> +        pl011_tx_loopback(s, ch);

This implementation will send the transmitted characters
to the QEMU chardev and also loop them back into the UART
when loopback is enabled. Similarly if we receive a character
from the real input we will put it into the FIFO still, so
the FIFO will get both looped-back and real input together.

I think we only have one other UART where loopback is implemented,
and that is hw/char/serial.c. In that device we make loopback not
send transmitted characters out when in loopback mode, because
the 16550 datasheet explicitly says that's how its loopback
mode works. The PL011 datasheet is unfortunately silent on
this question. Do you have a real hardware PL011 that you
can check to see whether when it is in loopback mode
transmitted data is also sent to the output port as well
as looped back? Similarly for input: we should check whether
the UART continues to accept real input or if the real input
is completely disconnected while in loopback mode.

>          pl011_update(s);
>          break;
>      case 1: /* UARTRSR/UARTECR */
> @@ -300,8 +346,9 @@ static void pl011_write(void *opaque, hwaddr offset,
>          pl011_set_read_trigger(s);
>          break;
>      case 12: /* UARTCR */
> -        /* ??? Need to implement the enable and loopback bits.  */
> +        /* ??? Need to implement the enable bit.  */
>          s->cr = value;
> +        pl011_cr_loopback(s, true);
>          break;
>      case 13: /* UARTIFS */
>          s->ifl = value;
> --

thanks
-- PMM