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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]