On 03/05/2019 09:14, Mark Cave-Ayland wrote:
> On 02/05/2019 13:11, Philippe Mathieu-Daudé wrote:
>
>> On 5/2/19 11:04 AM, Laurent Vivier wrote:
>>> On 19/04/2019 17:40, 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 behavior was triggered by firmware for an embedded system with a
>>>> Z85C30 which necessitated this patch.
>>>>
>>>> This patch fixes that situation by explicitly lowering the IRQ when a
>>>> character is written to the buffer and no other interrupts are currently
>>>> pending.
>>>>
>>>> Signed-off-by: Stephen Checkoway <stephen.checko...@oberlin.edu>
>>>> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>>>> ---
>>>>
>>>> I added a sentence about the Z85C30 necessitating this to the commit
>>>> message.
>>>>
>>>> hw/char/escc.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/hw/char/escc.c b/hw/char/escc.c
>>>> index 628f5f81f7..c5b05a63f1 100644
>>>> --- a/hw/char/escc.c
>>>> +++ b/hw/char/escc.c
>>>> @@ -509,6 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>>>> break;
>>>> case SERIAL_DATA:
>>>> trace_escc_mem_writeb_data(CHN_C(s), val);
>>>> + /*
>>>> + * Lower the irq when data is written to the Tx buffer and no
>>>> other
>>>> + * interrupts are currently pending. The irq will be raised again
>>>> once
>>>> + * the Tx buffer becomes empty below.
>>>> + */
>>>> + s->txint = 0;
>>>> + escc_update_irq(s);
>>>> s->tx = val;
>>>> if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
>>>> if (qemu_chr_fe_backend_connected(&s->chr)) {
>>>>
>>>
>>>
>>> Applied to my trivial-patches branch.
>>
>> Mark, Artyom, are you OK with this patch?
>
> I started testing this with my OpenBIOS test images at the start of the week,
> but
> unfortunately got distracted by real life :)
>
> I've now finished and confirmed there are no regressions in my local tests,
> so I'll
> include this in the PR I am planning to send shortly containing the leon3
> updates.
Hi Mark,
I've already included a leon3 patch ("hw/sparc/leon3: Allow load of
uImage firmwares") in the PR I sent yesterday for the trivial branch.
I've removed from my PR this patch about the ESCC, so you can take it.
Thanks,
Laurent
