Re: [Qemu-trivial] [Qemu-devel] [PATCH] hw/char/escc: Lower irq when transmit buffer is filled

[Philippe Mathieu-Daudé]

On 3/6/19 12:01 PM, Paolo Bonzini wrote:
> On 05/03/19 06:10, Stephen Checkoway wrote:
>> The SCC/ESCC will briefly stop asserting an interrupt when the
>> transmit FIFO is filled.
>>
>> This code doesn't model the transmit FIFO/shift register so the
>> pending transmit interrupt is never deasserted which means that an
>> edge-triggered interrupt controller will never see the low-to-high
>> transition it needs to raise another interrupt. The practical
>> consequence of this is that guest firmware with an interrupt service
>> routine for the ESCC that does not send all of the data it has
>> immediately will stop sending data if the following sequence of
>> events occurs:
>> 1. Disable processor interrupts
>> 2. Write a character to the ESCC
>> 3. Add additional characters to a buffer which is drained by the ISR
>> 4. Enable processor interrupts
>>
>> In this case, the first character will be sent, the interrupt will
>> fire and the ISR will output the second character. Since the pending
>> transmit interrupt remains asserted, no additional interrupts will
>> ever fire.
>>
>> This fixes that situation by explicitly lowering the IRQ when a
>> character is written to the buffer.
>>
>> Signed-off-by: Stephen Checkoway <address@hidden>
> 
> Looks good but I would like Mark to give his ack as well.
> 
> Mark, could you also add hw/char/escc.c to both SPARC and Mac sections
> of MAINTAINERS?

Cc'ing Artyom who also made some changes in this file, and Laurent.


Stephen, which particular chipset are you using?

This file models various types. I had a talk with Mark or Laurent at
last KVM forum about this device. IIRC, while the sun4m machines use a
real chipset, the MacIO embeds an slighly modified IP core.

I can't find what you describe in the Z85C30 doc, however in the ESCC
datasheet referenced in escc.c (which happens to be a 85MiB pdf!!!) I found:

  Transmit Interrupts and Transmit Buffer Empty Bit on the NMOS/CMOS

  The TxIP is reset either by writing data to the transmit buffer or
  by issuing the Reset Tx Int command in WR0.

I understand the NMOS/CMOS desc. matches the Z85C30 (no FIFO used).

So your description and patch makes sens.
What worries me is the controller could have other pending IRQs to
deliver and you are clearing them. Shouldn't we only clear the
INTR_TXINT bit, and call escc_update_irq() which should lower the IRQ if
no bits are pending?

Maybe as:

    s->wregs[W_INTR] &= ~INTR_TXINT;
    escc_update_irq(s);

Thanks,

Phil.

> Thanks,
> 
> Paolo
> 
>> ---
>>  hw/char/escc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/char/escc.c b/hw/char/escc.c
>> index 628f5f81f7..bea55ad8da 100644
>> --- a/hw/char/escc.c
>> +++ b/hw/char/escc.c
>> @@ -509,6 +509,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>>          break;
>>      case SERIAL_DATA:
>>          trace_escc_mem_writeb_data(CHN_C(s), val);
>> +        qemu_irq_lower(s->irq);
>>          s->tx = val;
>>          if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
>>              if (qemu_chr_fe_backend_connected(&s->chr)) {
>>
> 
>