[Qemu-arm] [PATCH v2 2/3] hw/intc: Add (new) ASPEED AST2400 AVIC device model

[Peter Maydell]

On 3 March 2016 at 05:14, Andrew Jeffery <andrew at aj.id.au> wrote:
> On Thu, 2016-02-25 at 16:29 +0000, Peter Maydell wrote:
>> > +    case 0x20: /* Interrupt Enable */
>> > +        s->int_enable |= data;
>>
>> Are you sure this only ORs in new 1 bits?
>
> As in, am I sure I only want to take the newly set bits? If so, yes, as
> the the following register serves to clear the field's set bits:
>
>>
>> > +        break;
>> > +    case 0x28: /* Interrupt Enable Clear */
>> > +        s->int_enable &= ~data;
>> > +        break;
>
> The 'int_enable', 'int_trigger' and 'edge_status' fields all use the pa
> ttern of separate set and clear registers (the remaining registers may
> benefit from the extract64/deposit64 helpers, I'll think about that
> further). I'll add some comments to help clear this up.
>
> Otherwise, can you rephrase the question? At face value it seems like
> you're implying that I'm doing more than ORing in the new 1 bits?

It was just that the register name didn't imply a set-bits-only
semantic and some of the other registers looked like they were
also incorrectly not handling updates right.

thanks
-- PMM