[Qemu-devel] [PATCH] target-ppc: Fix openpic timer read register offset

[Qemu-devel] [PATCH] target-ppc: Fix openpic timer read register offset

[Aaron Larson]

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(-)

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;
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target-ppc: Fix openpic timer read register offset

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3478 bytes --]

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			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] target-ppc: Fix openpic timer read register offset

[alarson]

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.