Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of
> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h 
> b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..9c012e6c977 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -30,7 +30,7 @@
>
>  #include "hw/char/serial.h"
>
> -#define MCHP_PFSOC_MMUART_REG_SIZE  52
> +#define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
>      MemoryRegion iomem;
> @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
>
>      SerialMM *serial;
>
> -    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
> +    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
>
>  /**
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..584e7fec17c 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, 
> hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr << 2);
>          return 0;
>      }
>
> -    return s->reg[addr / sizeof(uint32_t)];
> +    return s->reg[addr];
>  }
>
>  static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
> @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr 
> addr,
>      MchpPfSoCMMUartState *s = opaque;
>      uint32_t val32 = (uint32_t)value;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> -                      " v=0x%x\n", __func__, addr, val32);
> +                      " v=0x%x\n", __func__, addr << 2, val32);
>          return;
>      }
>
> -    s->reg[addr / sizeof(uint32_t)] = val32;
> +    s->reg[addr] = val32;
>  }
>
>  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> --
> 2.31.1
>