On Fri, 2016-03-11 at 16:03 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <and...@aj.id.au> wrote:
> > Implement a basic ASPEED VIC device model, enough to boot a Linux kernel
> > configured with aspeed_defconfig. The model implements the 'new'
> > (revised) register set and while the hardware exposes both the new and
> > legacy register sets, accesses to the legacy register set will not
> > be serviced (though the access will be logged).
> >
> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
>
> > +static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
> > + unsigned size)
> > +{
> > + const bool high = !!(offset & 0x4);
> > + hwaddr n_offset = (offset & ~0x4);
> > + AspeedVICState *s = (AspeedVICState *)opaque;
> > +
> > + if (offset < AVIC_NEW_BASE_OFFSET) {
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: Ignoring write to legacy registers at 0x%"
> > + HWADDR_PRIx "[%u] <- 0x%" PRIx64 "\n", __func__,
> > offset,
> > + size, data);
> > + return;
> > + }
> > +
> > + n_offset -= AVIC_NEW_BASE_OFFSET;
> > + trace_aspeed_vic_write(offset, size, data);
> > +
> > + /* Given we have members using separate enable/clear registers,
> > deposit64()
> > + * isn't quite the tool for the job. Instead, relocate the incoming
> > bits to
> > + * the required bit offset based on the provided access address
> > + */
> > + if (high) {
> > + data &= AVIC_H_MASK;
> > + data <<= 32;
> > + } else {
> > + data &= AVIC_L_MASK;
> > + }
> > +
> > + switch (n_offset) {
> > + case 0x18: /* Interrupt Selection */
> > + /* Register has deposit64() semantics - overwrite requested 32
> > bits */
> > + if (high) {
> > + s->select &= AVIC_L_MASK;
> > + } else {
> > + s->select &= ((uint64_t) AVIC_H_MASK) << 32;
> > + }
> > + s->select |= data;
> > + break;
> > + case 0x20: /* Interrupt Enable */
> > + s->enable |= data;
> > + break;
> > + case 0x28: /* Interrupt Enable Clear */
> > + s->enable &= ~data;
> > + break;
> > + case 0x30: /* Software Interrupt */
> > + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > + "IRQs requested: 0x%016" PRIx64 "\n", __func__, data);
> > + break;
> > + case 0x38: /* Software Interrupt Clear */
> > + qemu_log_mask(LOG_UNIMP, "%s: Software interrupts unavailable. "
> > + "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
> > + break;
> > + case 0x50: /* Interrupt Event */
> > + /* Register has deposit64() semantics - overwrite the top four
> > valid
> > + * IRQ bits, as only the top four IRQs (GPIOs) can change their
> > event
> > + * type */
> > + g_assert(high);
>
> Don't assert on conditions that can be triggered by a guest.Good point, I'll change it to qemu_log_mask(LOG_GUEST_ERROR, ...) > > > + s->event &= ~AVIC_EVENT_W_MASK; > > + s->event |= (data & AVIC_EVENT_W_MASK); > > + break; > > + case 0x58: /* Edge Triggered Interrupt Clear */ > > + s->raw &= ~(data & ~s->sense); > > + break; > > + case 0x00: /* IRQ Status */ > > + case 0x08: /* FIQ Status */ > > + case 0x10: /* Raw Interrupt Status */ > > + case 0x40: /* Interrupt Sensitivity */ > > + case 0x48: /* Interrupt Both Edge Trigger Control */ > > + case 0x60: /* Edge Triggered Interrupt Status */ > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Write of read-only register with offset 0x%" > > + HWADDR_PRIx "\n", __func__, offset); > > + break; > > + > > + default: > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Bad register at offset 0x%" HWADDR_PRIx "\n", > > + __func__, offset); > > + break; > > + } > > + aspeed_vic_update(s); > > +} > > Otherwise > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> Thanks, Andrew > > thanks > -- PMM
signature.asc
Description: This is a digitally signed message part
