Re: [Qemu-devel] [PATCH 1/2] hw/armv7m_nvic: Implement byte read/write for NVIC SCB_SHPRx registers

[Peter Maydell]

On 9 October 2012 23:29, Andre Beckus <address@hidden> wrote:
> Adds nvic_writeb and nvic_readb functions.
>
> Implements byte read/write for the NVIC SCB_SHPRx (System Handler
> Priority Registers).  Currently, only double word access is implemented.
> The ARM CMSIS library maps these registers to a byte array, which requires
> that byte access be implemented.
>
> Note that because the NVIC ID register read handles both byte and word reads,
> it is left as-is, and not moved into the new nvic_readb function.
>
> Signed-off-by: Andre Beckus <address@hidden>

Hi Andre. Thanks for this patch -- it's certainly much easier and cleaner
looking to do this byte access handling now we've managed to separate the
system control registers out of the gic registers. Some points below which
hopefully should be straightforward to fix...

> ---
>  hw/armv7m_nvic.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
> index 5c09116..10b5954 100644
> --- a/hw/armv7m_nvic.c
> +++ b/hw/armv7m_nvic.c
> @@ -138,6 +138,26 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
>      gic_complete_irq(&s->gic, 0, irq);
>  }
>
> +static uint32_t nvic_readb(void *opaque, uint32_t offset)
> +{
> +    nvic_state *s = (nvic_state *)opaque;
> +    uint32_t val;
> +    int irq;
> +
> +    if (offset < 0xd18) {
> +        goto bad_reg;
> +    } else if (offset < 0xd24) {
> +        irq = offset - 0xd14;
> +        val = s->gic.priority1[irq][0];
> +    } else {
> +        goto bad_reg;
> +    }

I think it would be cleaner to use switch here. You can use
the 0xnn ... 0xnn range syntax, eg

    switch (offset) {
    case 0xd18 ... 0xd24:
        return s->gic.priority1[offset - 0xd14][0];
    default:
        hw_error(...);
    }

(makes it easy to later add the other set of registers which
are byte accessible, which is the fault status registers.)

> +    return val;
> +bad_reg:
> +    hw_error("NVIC: Bad read offset 0x%x\n", offset);
> +    return 0;

You don't need this "return 0" after a hw_error() because hw_error()
will never return.

> +}
> +
>  static uint32_t nvic_readl(void *opaque, uint32_t offset)
>  {
>      nvic_state *s = (nvic_state *)opaque;
> @@ -285,6 +305,25 @@ static uint32_t nvic_readl(void *opaque, uint32_t offset)
>      }
>  }
>
> +static void nvic_writeb(void *opaque, uint32_t offset, uint32_t value)
> +{
> +    nvic_state *s = (nvic_state *)opaque;
> +    int irq;
> +
> +    if (offset < 0xd18) {
> +        goto bad_reg;
> +    } else if (offset < 0xd24) {
> +        irq = offset - 0xd14;
> +        s->gic.priority1[irq][0] = value;
> +        gic_update(&s->gic);
> +    } else {
> +        goto bad_reg;
> +    }
> +    return;
> +bad_reg:
> +    hw_error("NVIC: Bad read offset 0x%x\n", offset);

Similar comments as above about using switch().

> +}
> +
>  static void nvic_writel(void *opaque, uint32_t offset, uint32_t value)
>  {
>      nvic_state *s = (nvic_state *)opaque;
> @@ -408,6 +447,8 @@ static uint64_t nvic_sysreg_read(void *opaque, 
> target_phys_addr_t addr,
>      }
>      if (size == 4) {
>          return nvic_readl(opaque, offset);
> +    } else if (size == 1) {
> +        return nvic_readb(opaque, offset);

The SHPR registers are also halfword accessible, so it would be nice
to complete the set by adding nvic_readw and nvic_writew.


>      }
>      hw_error("NVIC: Bad read of size %d at offset 0x%x\n", size, offset);
>  }
> @@ -419,6 +460,9 @@ static void nvic_sysreg_write(void *opaque, 
> target_phys_addr_t addr,
>      if (size == 4) {
>          nvic_writel(opaque, offset, value);
>          return;
> +    } else if (size == 1) {
> +        nvic_writeb(opaque, offset, value);
> +        return;
>      }
>      hw_error("NVIC: Bad write of size %d at offset 0x%x\n", size, offset);
>  }
> --
> 1.7.9.5
>

-- PMM