[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Mark Cave-Ayland
Subject: Re: [PATCH] escc: update transmit status bits when switching to async mode
Date: Thu, 11 Nov 2021 17:36:40 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

On 02/11/2021 14:46, Peter Maydell wrote:

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 

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)

The wording is certainly confusing but the description on p.177 does state "This bit is set to 1 when the transmit buffer is empty". My impression from reading the initial paragraph is that the bit is reset to 0 whenever a character is added into the TX FIFO suggesting that if this bit is set to 1 then the TX FIFO is empty.

  * in sync mode, ALLSENT is always high

Yes, agreed.

  * 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")

When I wrote the patch I was assuming that this might not be the case for escc_kbd and escc_mouse but I think you could be right here.

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

Again the wording isn't particularly helpful here but I think what it is trying to say is that TXEMPTY remains reset to 0, even when the TX FIFO is empty, until after the hardware-generated CRC has been shifted out onto the wire.

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;
          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
  * 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

  * 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;

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

instead of the /* fallthrough */.

I'm starting to wonder if asserting the reset signal does do the same as setting the hardware reset bit, since this would then enable the Sun PROM to pass the STATUS_TXEMPTY check. This would just leave the SPEC_ALLSENT bit which would be updated on a write to W_TXCTRL1 to pass the second check.

Assuming that QEMU will always accept the data then I think it would be a case of setting SPEC_ALLSENT regardless, since if there is no delay in transmitting the data then a transition of SPEC_ALLSENT will never be visible to the guest.

Certainly we need a solution for 6.2 since the majority of qemu-system-sparc users who boot Solaris use the real Sun PROM (many of the online tutorials were written when Artyom first merged his changes which was before OpenBIOS could boot Solaris).



reply via email to

[Prev in Thread] Current Thread [Next in Thread]