On Mon, Apr 10, 2017 at 5:44 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 5 April 2017 at 00:40, Alistair Francis <alistair.fran...@xilinx.com>
> wrote:
>> Only trigger multi-queue GEM interrupts if the interrupt status register
>> is set. This logic was already used for non multi-queue interrupts but
>> it also applies to multi-queue interrupts.
>>
>> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
>> ---
>>
>> hw/net/cadence_gem.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 3e37665..b9eaed4 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -518,7 +518,7 @@ static void gem_update_int_status(CadenceGEMState *s)
>> }
>>
>> for (i = 0; i < s->num_priority_queues; ++i) {
>> - if (s->regs[GEM_INT_Q1_STATUS + i]) {
>> + if (s->regs[GEM_INT_Q1_STATUS + i] && s->regs[GEM_ISR]) {
>> DB_PRINT("asserting int. (q=%d)\n", i);
>> qemu_set_irq(s->irq[i], 1);
>> }
>
> With this change, if s->regs[GEM_ISR] is zero then the entire
> function never does anything, so you could hoist that up to
> the start, rather than testing it inside the loop and in the
> previous conditional.
Yep, I have moved it to the top.
>
> Also the comment says "raise or lower interrupt based on current
> status", but the code will only ever do qemu_set_irq(..., 1),
> never 0. Which is right?
This is a little confusing. The interrupts are lowered when the ISR is
read, so the assumption was that we never need to clear them in the
gem_update_int_status(). Although then if we perform a reset nothing
will clear the interrupts until the ISR is read from. So that is a
bug. I'll update this patch to fix this up in V2.
Thanks,
Alistair
>
> thanks
> -- PMM
