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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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

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

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


ATB,

Mark.