Re: [PATCH] escc: update transmit status bits when switching to async mode

[Peter Maydell]

On Mon, 1 Nov 2021 at 20:31, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> The recent ESCC reset changes cause a regression when attemping to use a real
> SS-5 Sun PROM instead of OpenBIOS. The Sun PROM doesn't send an explicit reset
> command to the ESCC but gets stuck in a loop probing the keyboard waiting for
> STATUS_TXEMPTY to be set in R_STATUS followed by SPEC_ALLSENT in R_SPEC.
>
> Reading through the ESCC datasheet again indicates that SPEC_ALLSENT is 
> updated
> when a write to W_TXCTRL1 selects async mode, or remains high if sync mode is
> selected. Whilst there is no explicit mention of STATUS_TXEMPTY, the ESCC 
> device
> emulation always updates these two register bits together when transmitting 
> data.

My reading of the spec is that this isn't the right behaviour. I think
what we ought to have is:
 * in both async and sync mode, TXEMPTY tracks the tx fifo status
   (which I think means "set if there is any space in it", contrary to
   what the name of the bit implies)
 * in sync mode, ALLSENT is always high
 * in async mode, ALLSENT reads the same as TXEMPTY (for us, since we have
   no extra delay between "data left the FIFO" and "data actually on the wire")
 * in sync mode TXEMPTY is apparently also set "when the last bit of the CRC
   has cleared the transmit shift register". I don't think I really understand
   what sync mode TXEMPTY does overall, but clearly it is not "always 0".

> Add extra logic to update both transmit status bits accordingly when writing 
> to
> W_TXCTRL1 which enables the Sun PROM to initialise and boot again under QEMU.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/char/escc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 0fce4f6324..b33cdc229f 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -575,6 +575,18 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>              s->wregs[s->reg] = val;
>              break;
>          case W_TXCTRL1:
> +            s->wregs[s->reg] = val;
> +            if (val & TXCTRL1_STPMSK) {
> +                ESCCSERIOQueue *q = &s->queue;
> +                if (s->type == escc_serial || q->count == 0) {
> +                    s->rregs[R_STATUS] |= STATUS_TXEMPTY;
> +                    s->rregs[R_SPEC] |= SPEC_ALLSENT;
> +                }

...should there be an 'else' clause here which clears these
bits if we are now in async mode and the tx queue is not empty ?

> +            } else {
> +                s->rregs[R_STATUS] &= ~STATUS_TXEMPTY;
> +                s->rregs[R_SPEC] |= SPEC_ALLSENT;
> +            }

Something is a bit odd with how we currently handle both these bits
For SPEC_ALLSENT:
 * it is zero on power-on
 * soft-reset leaves it unchanged
 * we set it on a write to SERIAL_DATA
 * this new code adds two places where we set it
 * but there is nowhere where we clear it

For STATUS_TXEMPTY:
 * it is set on power-on and soft-reset
 * it is set on write to SERIAL_DATA
 * it is never cleared

Shouldn't we be clearing these bits somewhere ?

> +            /* fallthrough */
>          case W_TXCTRL2:
>              s->wregs[s->reg] = val;
>              escc_update_parameters(s);

At this point all the "fallthrough" is doing is (1) repeat the
setting of s->wregs[s->reg] and (2) call escc_update_parameters().
So I think it would be clearer to instead make the two cases
completely separate, and have
   escc_update_parameters(s);
   break;

instead of the /* fallthrough */.

-- PMM