Re: [PATCH v2 10/17] hw/misc: Support 8-bytes memop in NPCM GCR module

[Peter Maydell]

On Thu, 26 Dec 2024 at 08:28, Hao Wu <wuhaotsh@google.com> wrote:
>
> The NPCM8xx GCR device can be accessed with 64-bit memory operations.
> This patch supports that.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  hw/misc/npcm_gcr.c   | 94 +++++++++++++++++++++++++++++++++-----------
>  hw/misc/trace-events |  4 +-
>  2 files changed, 74 insertions(+), 24 deletions(-)
>
> diff --git a/hw/misc/npcm_gcr.c b/hw/misc/npcm_gcr.c
> index a4c9643119..7dfdd3d74b 100644
> --- a/hw/misc/npcm_gcr.c
> +++ b/hw/misc/npcm_gcr.c
> @@ -201,6 +201,7 @@ static uint64_t npcm_gcr_read(void *opaque, hwaddr 
> offset, unsigned size)
>      uint32_t reg = offset / sizeof(uint32_t);
>      NPCMGCRState *s = opaque;
>      NPCMGCRClass *c = NPCM_GCR_GET_CLASS(s);
> +    uint64_t value;
>
>      if (reg >= c->nr_regs) {
>          qemu_log_mask(LOG_GUEST_ERROR,
> @@ -209,9 +210,21 @@ static uint64_t npcm_gcr_read(void *opaque, hwaddr 
> offset, unsigned size)
>          return 0;
>      }
>
> -    trace_npcm_gcr_read(offset, s->regs[reg]);
> +    switch (size) {
> +    case 4:
> +        value = s->regs[reg];
> +        break;
> +
> +    case 8:
> +        value = s->regs[reg] + (((uint64_t)s->regs[reg + 1]) << 32);

Maybe
  value = deposit64(s->regs[reg], 32, 32, s->regs[reg + 1]);
?

(Mostly it's just the cast that makes me suggest a rephrase.)

> +        break;
> +
> +    default:
> +        g_assert_not_reached();
> +    }
>
> -    return s->regs[reg];
> +    trace_npcm_gcr_read(offset, value);
> +    return value;
>  }
>
>  static void npcm_gcr_write(void *opaque, hwaddr offset,
> @@ -231,29 +244,65 @@ static void npcm_gcr_write(void *opaque, hwaddr offset,
>          return;
>      }
>
> -    switch (reg) {
> -    case NPCM7XX_GCR_PDID:
> -    case NPCM7XX_GCR_PWRON:
> -    case NPCM7XX_GCR_INTSR:
> -        qemu_log_mask(LOG_GUEST_ERROR,
> -                      "%s: register @ 0x%04" HWADDR_PRIx " is read-only\n",
> -                      __func__, offset);
> -        return;
> -
> -    case NPCM7XX_GCR_RESSR:
> -    case NPCM7XX_GCR_CP2BST:
> -        /* Write 1 to clear */
> -        value = s->regs[reg] & ~value;
> +    switch (size) {
> +    case 4:
> +        switch (reg) {
> +        case NPCM7XX_GCR_PDID:
> +        case NPCM7XX_GCR_PWRON:
> +        case NPCM7XX_GCR_INTSR:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: register @ 0x%04" HWADDR_PRIx " is 
> read-only\n",
> +                          __func__, offset);
> +            return;
> +
> +        case NPCM7XX_GCR_RESSR:
> +        case NPCM7XX_GCR_CP2BST:
> +            /* Write 1 to clear */
> +            value = s->regs[reg] & ~value;
> +            break;
> +
> +        case NPCM7XX_GCR_RLOCKR1:
> +        case NPCM7XX_GCR_MDLR:
> +            /* Write 1 to set */
> +            value |= s->regs[reg];
> +            break;
> +        };
> +        s->regs[reg] = value;
>          break;
>
> -    case NPCM7XX_GCR_RLOCKR1:
> -    case NPCM7XX_GCR_MDLR:
> -        /* Write 1 to set */
> -        value |= s->regs[reg];
> +    case 8:
> +        s->regs[reg] = value;
> +        s->regs[reg + 1] = v >> 32;

s->regs[reg + 1] = extract64(v, 32, 32);

>          break;
> -    };
>
> -    s->regs[reg] = value;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM