Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour

[Li Guang <lig.fnst@cn.fujitsu.com>]

Beniamino Galvani wrote:
> On Mon, Feb 24, 2014 at 02:45:06PM +0800, Li Guang wrote:
>    
>> Beniamino Galvani wrote:
>>      
>>> On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote:
>>>        
>>>> Beniamino Galvani wrote:
>>>>          
>>>>> On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote:
>>>>>            
>>>>>> Beniamino Galvani wrote:
>>>>>>              
>>>>>>> According to this mail thread [1], writing to pending register seems
>>>>>>> to have no effect on actual pending status of interrupts. This means
>>>>>>> that the only way to clear a pending interrupt is to clear the
>>>>>>> interrupt source. This patch implements such behaviour.
>>>>>>>
>>>>>>> [1] http://lkml.org/lkml/2013/7/6/59
>>>>>>>
>>>>>>> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
>>>>>>> ---
>>>>>>>   hw/intc/allwinner-a10-pic.c |    6 ++++--
>>>>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
>>>>>>> index bb2351f..afd57ef 100644
>>>>>>> --- a/hw/intc/allwinner-a10-pic.c
>>>>>>> +++ b/hw/intc/allwinner-a10-pic.c
>>>>>>> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level)
>>>>>>>
>>>>>>>       if (level) {
>>>>>>>           set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>>>> +    } else {
>>>>>>> +        clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]);
>>>>>>>       }
>>>>>>>       aw_a10_pic_update(s);
>>>>>>>   }
>>>>>>> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value,
>>>>>>>           s->nmi = value;
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8:
>>>>>>> -        s->irq_pending[index]&= ~value;
>>>>>>> +        /* Nothing to do */
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8:
>>>>>>> -        s->fiq_pending[index]&= ~value;
>>>>>>> +        /* Ditto */
>>>>>>>           break;
>>>>>>>       case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8:
>>>>>>>           s->select[index] = value;
>>>>>>>                
>>>>>> pending registers are also clear registers by a10 datasheet,
>>>>>> also you found bits are marked as 'R', so, ..., contradict itself.
>>>>>>              
>>>>> Yes, the datasheet is inconsistent about this because the register
>>>>> can't be read-only and 'clear' at the same time.
>>>>>
>>>>> Unfortunately at the moment I cannot test if the clearing
>>>>> functionality of the pending register works on real hardware but the
>>>>> idea I got from the linked discussion is that it's either not
>>>>> implemented or broken and therefore interrupts remain pending until
>>>>> they are disabled at the source.
>>>>>
>>>>> Do you have a chance to try it on a real board?
>>>>>
>>>>>            
>>>> Ah? even kernel code from allwinner wrote pending registers
>>>> to clear pending interrupt, didn't you see it?
>>>> so should be no doubt that these registers are writable.
>>>>          
>>> Well, if you look closely at that code, it's a bit strange:
>>>
>>> void sw_irq_ack(struct irq_data *irqd)
>>> {
>>>      unsigned int irq = irqd->irq;
>>>
>>>      [...]
>>>      writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<<irq), SW_INT_IRQ_PENDING_REG0);
>>>      [...]
>>> }
>>>
>>> In order to clear a single interrupt, it is or'ing the irq bit with
>>> the previous value of the register, so it's basically clearing all
>>> pending interrupts.
>>>
>>> This is discussed in the mentioned thread from lkml and the final
>>> explanation is that the pending register is always updated with the
>>> actual status of irq lines, even if you write to it. In other words,
>>> writes to the register are ignored, otherwise the code above would
>>> produce random loss of interrupts.
>>>
>>>        
>> Hmm..., sorry,  I also can't test this operation on A10 board now,
>> but why not they just wipe out these writings(kernel 3.12)?
>>      
> I don't know, there was a proposed patch that removed those writes in
> the lkml discussion but probably it never reached mainline.
>
> To be on the safe side I can restore the writability of the register
> and then, when we will figure out how it really works, correct it if
> needed.
>
>
>    

Agreed,

Thanks!