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

[Andre Beckus]

On Wed, 2012-10-10 at 12:32 +0100, Peter Maydell wrote:
> 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...

I agree it is cleaner.  I originally made these changes in the old
arm_gic code, and it was tricky with all of the #define's.

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

Will do.  I had used the arm_gic routines as a template, but prefer the
switch as well.  I also was not familiar with the range syntax until
now.

> > +    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.

Yes, I was being lazy.  Now that I think about it, we could handle all
sizes with one block of code directly in the nvic_sysreg_read and
nvic_sysreg_write functions - the write would look like this:

    for(i = 0; i < size; i++) {
        s->gic.priority1[(offset - 0xd14) + i][0] =
            (value >> (i * 8)) & 0xff;
    }

Then the writeb and readb functions would not be necessary and the SHPR
code could be removed from the writel and readl functions.  What do you
think?  Or is the goal to keep each access size isolated to its own
function?

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

Thank you,
Andre Beckus