[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device mod
From: |
Andrew Jeffery |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/3] hw/intc: Add (new) ASPEED VIC device model |
Date: |
Sat, 12 Mar 2016 13:54:25 +1030 |
On Fri, 2016-03-11 at 16:03 +0700, Peter Maydell wrote:
> On 5 March 2016 at 11:29, Andrew Jeffery <address@hidden> 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 <address@hidden>
>
> > +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 <address@hidden>
Thanks,
Andrew
>
> thanks
> -- PMM
signature.asc
Description: This is a digitally signed message part