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

[Stephen Checkoway]

On Apr 10, 2019, at 16:01, Philippe Mathieu-Daudé <address@hidden> wrote:

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

I'm away from the hardware, but my notes say Z80C30. I've included the image 
from the board if you want to try to decipher the rest of the part number. My 
best guess is it's a Z85C3008VEC. If it's important, I can ask someone who is a 
few thousand km closer to the hardware than I currently am to take a look.



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

(This chip is ridiculously configurable, I honestly found it much easier to 
write a driver by reverse engineering existing firmware than by reading the 
part sheet.)

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

It has been a little while since I was last looking at this, but my 
recollection was the SCC has a 1-byte transmit buffer and the ESCC has a small 
(4 maybe?) byte transmit buffer. But in either case writing data to the 
transmit buffer should clear the interrupt.

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

Ah, I think I see. In this case, escc_update_irq() will lower the IRQ if no 
other interrupts are pending and the set_txint() will raise it again. 
Otherwise, it'll remain raised. I can give this a shot and see how it goes. (I 
think this should be R_INTR instead of W_INTR, but I'll double check once I 
have an opportunity to dig into this again.)

Thanks for looking at this!

Steve


-- 
Stephen Checkoway