Message ID | 201706021132.v52BWx1t027533@linux03a.ddci.com |
---|---|
State | New |
Headers | show |
On Fri, Jun 02, 2017 at 04:32:59AM -0700, Aaron Larson wrote: > > openpic_tmr_read() is incorrectly computing register offset of the > TCCR, TBCR, TVPR, and TDR registers when accessing the open pic timer > registers. Specifically the offset of timer registers for > openpic_tmr_read() is not accounting for the timer frequency reporting > register (TFFR) which is the first register in the "tmr" memory > region. > > openpic_tmr_write() *is* correctly computing the offset by adding > 0x10f0 to the address prior to computing the register index. This > patch instead subtracts 0x10 in both the read and write routines and > eliminates some other gratuitous differences between the functions. > > Signed-off-by: Aaron Larson <alarson@ddci.com> Applied to ppc-for-2.10. It looks saner than the existing code, but I don't know openpic well enough to really review it; so I'm trusting you on that. > --- > hw/intc/openpic.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c > index 4349e45..f966d06 100644 > --- a/hw/intc/openpic.c > +++ b/hw/intc/openpic.c > @@ -796,27 +796,24 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len) > } > > static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val, > - unsigned len) > + unsigned len) > { > OpenPICState *opp = opaque; > int idx; > > - addr += 0x10f0; > - > DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n", > - __func__, addr, val); > + __func__, (addr + 0x10f0), val); > if (addr & 0xF) { > return; > } > > - if (addr == 0x10f0) { > + if (addr == 0) { > /* TFRR */ > opp->tfrr = val; > return; > } > - > + addr -= 0x10; /* correct for TFRR */ > idx = (addr >> 6) & 0x3; > - addr = addr & 0x30; > > switch (addr & 0x30) { > case 0x00: /* TCCR */ > @@ -844,16 +841,17 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) > uint32_t retval = -1; > int idx; > > - DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr); > + DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0); > if (addr & 0xF) { > goto out; > } > - idx = (addr >> 6) & 0x3; > - if (addr == 0x0) { > + if (addr == 0) { > /* TFRR */ > retval = opp->tfrr; > goto out; > } > + addr -= 0x10; /* correct for TFRR */ > + idx = (addr >> 6) & 0x3; > switch (addr & 0x30) { > case 0x00: /* TCCR */ > retval = opp->timers[idx].tccr; > @@ -861,10 +859,10 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) > case 0x10: /* TBCR */ > retval = opp->timers[idx].tbcr; > break; > - case 0x20: /* TIPV */ > + case 0x20: /* TVPR */ > retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx); > break; > - case 0x30: /* TIDE (TIDR) */ > + case 0x30: /* TDR */ > retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx); > break; > }
David Gibson <david@gibson.dropbear.id.au> wrote on 06/04/2017 07:55:34 PM: > From: David Gibson <david@gibson.dropbear.id.au> > To: Aaron Larson <alarson@ddci.com> > Cc: agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org > Date: 06/04/2017 07:55 PM > Subject: Re: [PATCH] target-ppc: Fix openpic timer read register offset > > On Fri, Jun 02, 2017 at 04:32:59AM -0700, Aaron Larson wrote: > > > > openpic_tmr_read() is incorrectly computing register offset of the > > TCCR, TBCR, TVPR, and TDR registers when accessing the open pic timer > > registers. ... > > Applied to ppc-for-2.10. It looks saner than the existing code, but I > don't know openpic well enough to really review it; so I'm trusting > you on that. Thanks David, I'll have an updated timer patch out later today for the timer that actually times. I'm reasonably sure the resulting code is correct because we have the same binary working on real hardware and on QEMU.
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index 4349e45..f966d06 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -796,27 +796,24 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len) } static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val, - unsigned len) + unsigned len) { OpenPICState *opp = opaque; int idx; - addr += 0x10f0; - DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n", - __func__, addr, val); + __func__, (addr + 0x10f0), val); if (addr & 0xF) { return; } - if (addr == 0x10f0) { + if (addr == 0) { /* TFRR */ opp->tfrr = val; return; } - + addr -= 0x10; /* correct for TFRR */ idx = (addr >> 6) & 0x3; - addr = addr & 0x30; switch (addr & 0x30) { case 0x00: /* TCCR */ @@ -844,16 +841,17 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) uint32_t retval = -1; int idx; - DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr); + DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0); if (addr & 0xF) { goto out; } - idx = (addr >> 6) & 0x3; - if (addr == 0x0) { + if (addr == 0) { /* TFRR */ retval = opp->tfrr; goto out; } + addr -= 0x10; /* correct for TFRR */ + idx = (addr >> 6) & 0x3; switch (addr & 0x30) { case 0x00: /* TCCR */ retval = opp->timers[idx].tccr; @@ -861,10 +859,10 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) case 0x10: /* TBCR */ retval = opp->timers[idx].tbcr; break; - case 0x20: /* TIPV */ + case 0x20: /* TVPR */ retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx); break; - case 0x30: /* TIDE (TIDR) */ + case 0x30: /* TDR */ retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx); break; }
openpic_tmr_read() is incorrectly computing register offset of the TCCR, TBCR, TVPR, and TDR registers when accessing the open pic timer registers. Specifically the offset of timer registers for openpic_tmr_read() is not accounting for the timer frequency reporting register (TFFR) which is the first register in the "tmr" memory region. openpic_tmr_write() *is* correctly computing the offset by adding 0x10f0 to the address prior to computing the register index. This patch instead subtracts 0x10 in both the read and write routines and eliminates some other gratuitous differences between the functions. Signed-off-by: Aaron Larson <alarson@ddci.com> --- hw/intc/openpic.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)