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

[Andrew Jeffery]

On Thu, 2016-03-03 at 08:39 +0000, Peter Maydell wrote:
> On 3 March 2016 at 05:14, Andrew Jeffery <address@hidden> 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.

That's a fair call: The comments I added were the documented register
names, which don't provide huge insight on their own. This is probably
a less-than-ideal approach given the datasheet is largely unavailable.
As mentioned above I'll add some comments on the different access
patterns and where they apply to try clear this up.

Cheers,

Andrew

signature.asc
Description: This is a digitally signed message part