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>
---
 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;
+
+    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;
+}
+
 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);
         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;
-- 
2.25.1

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

On Thu, Feb 8, 2024 at 3:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:

> 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.

Hi Peter,

Here is what I found using hardware I have access to.

When loopback is enabled:

1. Receive is disconnected from the real input and
    only accepts transmit from loopback.

2. Transmitted characters is sent to both physical
    output and loopback to receive.

#2 is also collaborated by commit message for
   https://github.com/torvalds/linux/commit/734745ca

However, the same message also suggested that
#2 may not be the case in other implementations of pl011.

I will work on v2 to address you other comments
as well, with a property for customizing whether
transmit will send to both in loopback mode.

Regards,
Tong Ho

On Wed, 21 Feb 2024 at 06:56, Ho, Tong <tong.ho@amd.com> wrote:
>
> On Thu, Feb 8, 2024 at 3:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > 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.
>
> Hi Peter,
>
> Here is what I found using hardware I have access to.
>
> When loopback is enabled:
>
> 1. Receive is disconnected from the real input and
>     only accepts transmit from loopback.
>
> 2. Transmitted characters is sent to both physical
>     output and loopback to receive.
>
> #2 is also collaborated by commit message for
>    https://github.com/torvalds/linux/commit/734745ca
>
> However, the same message also suggested that
> #2 may not be the case in other implementations of pl011.
>
> I will work on v2 to address you other comments
> as well, with a property for customizing whether
> transmit will send to both in loopback mode.

Thanks for checking against the hardware behaviour.
I think that unless you have a need for both behaviours
in loopback mode, I would be happy to just implement the
same thing as the hardware you tested, and not worry about
adding the property.

-- PMM