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

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

[Andre Beckus]

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 <mikemail98-qemu@yahoo.com>
---
 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;
+    }
+    return val;
+bad_reg:
+    hw_error("NVIC: Bad read offset 0x%x\n", offset);
+    return 0;
+}
+
 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);
+}
+
 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);
     }
     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

[Qemu-devel] [PATCH 2/2] hw/armv7m_nvic: Add global variable for SysTick external reference clock

[Andre Beckus]

Adds a new external reference clock scale variable to complement the existing
system_clock_scale variable.  Previously, the value was hardcoded to 1000
when calculating the SysTick scale.  The new variable defaults to 1000 to
maintain backward compatibility.

Signed-off-by: Andre Beckus <mikemail98-qemu@yahoo.com>
---
 hw/arm-misc.h    |    4 ++++
 hw/armv7m_nvic.c |    3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/arm-misc.h b/hw/arm-misc.h
index bdd8fec..32b19df 100644
--- a/hw/arm-misc.h
+++ b/hw/arm-misc.h
@@ -65,4 +65,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
    ticks.  */
 extern int system_clock_scale;
 
+/* Multiplication factor to convert from external reference clock ticks to
+ * qemu timer ticks. */
+extern int external_ref_clock_scale;
+
 #endif /* !ARM_MISC_H */
diff --git a/hw/armv7m_nvic.c b/hw/armv7m_nvic.c
index 10b5954..4eaa113 100644
--- a/hw/armv7m_nvic.c
+++ b/hw/armv7m_nvic.c
@@ -65,6 +65,7 @@ static const uint8_t nvic_id[] = {
 #define SYSTICK_COUNTFLAG (1 << 16)
 
 int system_clock_scale;
+int external_ref_clock_scale = 1000;
 
 /* Conversion factor from qemu timer to SysTick frequencies.  */
 static inline int64_t systick_scale(nvic_state *s)
@@ -72,7 +73,7 @@ static inline int64_t systick_scale(nvic_state *s)
     if (s->systick.control & SYSTICK_CLKSOURCE)
         return system_clock_scale;
     else
-        return 1000;
+        return external_ref_clock_scale;
 }
 
 static void systick_reload(nvic_state *s, int reset)
-- 
1.7.9.5

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 <mikemail98-qemu@yahoo.com> 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 <mikemail98-qemu@yahoo.com>

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

Re: [Qemu-devel] [PATCH 2/2] hw/armv7m_nvic: Add global variable for SysTick external reference clock

[Peter Maydell]

On 9 October 2012 23:29, Andre Beckus <mikemail98-qemu@yahoo.com> wrote:
> Adds a new external reference clock scale variable to complement the existing
> system_clock_scale variable.  Previously, the value was hardcoded to 1000
> when calculating the SysTick scale.  The new variable defaults to 1000 to
> maintain backward compatibility.
>
> Signed-off-by: Andre Beckus <mikemail98-qemu@yahoo.com>

Hi. Do you have a use planned for this (new board maybe, or some
change to the stellaris board)? I generally prefer to put this kind
of patch in at the same time as the patches which make use of the
new feature, rather than putting them into the codebase without
any immediate users.

In this particular case, the system_clock_scale variable is a nasty
hack which I would like to get rid of. I think the right way to
implement the systick scaling/calibration is that instead of a
random global int, the NVIC exposes a GPIO input which corresponds
to the hardware's STCALIB input lines (which set the TENMS field
in SYST_CALIB), and the stellaris hardware sets that line via
qemu_set_irq().

I think that if the board provides an external reference clock,
then the scale should (as well as being used in the calculations,
as your patch does) appear in the return value form SYST_CALIB
(currently we return a hardcoded 10000 value.)

We should check what the stellaris code is doing when it sets
system_clock_scale -- I guess that's the actual system clock,
not the reference clock (if the stellaris even provides one).
The hardware doesn't have an explicit signal input to provide
that but we should probably fake one up to avoid the global.

-- PMM

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 <mikemail98-qemu@yahoo.com> 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 <mikemail98-qemu@yahoo.com>
> 
> 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

Re: [Qemu-devel] [PATCH 2/2] hw/armv7m_nvic: Add global variable for SysTick external reference clock

[Andre Beckus]

On Wed, 2012-10-10 at 13:03 +0100, Peter Maydell wrote:
> On 9 October 2012 23:29, Andre Beckus <mikemail98-qemu@yahoo.com> wrote:
> > Adds a new external reference clock scale variable to complement the existing
> > system_clock_scale variable.  Previously, the value was hardcoded to 1000
> > when calculating the SysTick scale.  The new variable defaults to 1000 to
> > maintain backward compatibility.
> >
> > Signed-off-by: Andre Beckus <mikemail98-qemu@yahoo.com>
> 
> Hi. Do you have a use planned for this (new board maybe, or some
> change to the stellaris board)? I generally prefer to put this kind
> of patch in at the same time as the patches which make use of the
> new feature, rather than putting them into the codebase without
> any immediate users.

No, this is for an STM32 implementation I wrote.  For the time being, I
am keeping it in a forked repository, and do not plan on submitting it -
the machine I made is based on a bare-bones dev board which has no
screen, etc..  I figured I would submit it if I ever add a more capable
machine that could, for example, run Linux and that might be of more
interest.

I had second thoughts after submitting this patch, and agree that it
should not be included.

> In this particular case, the system_clock_scale variable is a nasty
> hack which I would like to get rid of. I think the right way to
> implement the systick scaling/calibration is that instead of a
> random global int, the NVIC exposes a GPIO input which corresponds
> to the hardware's STCALIB input lines (which set the TENMS field
> in SYST_CALIB), and the stellaris hardware sets that line via
> qemu_set_irq().
> 
> I think that if the board provides an external reference clock,
> then the scale should (as well as being used in the calculations,
> as your patch does) appear in the return value form SYST_CALIB
> (currently we return a hardcoded 10000 value.)

I agree that GPIO pins would be a natural way to set the SYST_CALIB
register value.

> We should check what the stellaris code is doing when it sets
> system_clock_scale -- I guess that's the actual system clock,
> not the reference clock (if the stellaris even provides one).
> The hardware doesn't have an explicit signal input to provide
> that but we should probably fake one up to avoid the global.

I took a brief look at some Stellaris documentation, and it says the
Stellaris family does not include an external reference clock.
Furthermore, for at least some of the chips (and probably all of them),
the SYST_CALIB register is not implemented.

As a case study, the STM32 does have a reference clock.  It is simply
the system clock divided by 8 (maybe not ARM's intention for it to be
tied so closely to the system clock).  The documentation says the TENMS
field is hardwired to 9000, which corresponds to a 1 ms period when the
external reference clock is selected and the system clock is running at
72 Mhz.  So, the TENMS field will not be accurate if the system clock is
running at a different frequency (the SKEW bit is hardwired to 1).

Even if the SYST_CALIB were driven by GPIO lines, I think there would
still need to be a way of setting the frequency/scale of the two
sources, since they are not necessarily tied to the calibration value.  

Looking at the big picture, it seems that QEMU could benefit from a new
"clock line" type for handling clock signals.  They could be exposed by
devices in a similar manner to GPIO lines (there would be both input and
output clock lines).  I could see them being useful (at least in the
microcontroller world) for passing clock signals back and forth between
peripherals, interfacing timer peripherals to machines, setting
oscillator frequencies, and serving as the plumbing for clock trees.  I
know I had to do a lot of hacking with the STM32 implementation to
propagate the clock controller's signals to the other peripherals.

When I searched on the topic, I saw that you discussed/requested a
common clock framework back in July (in regards to the an exynos4210
patch).  Do you know if any progress was made?  As a side note, I had
made an attempt at a simple, generic clock tree interface for the STM32
implementation (it doesn't include clock lines that I mentioned above,
though).  It is not QOM - just a collection of C functions.  I've
included links to the code here:
https://github.com/beckus/qemu_stm32/blob/stm32/hw/clktree.h
https://github.com/beckus/qemu_stm32/blob/stm32/hw/clktree.c

Thank you,
Andre Beckus

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

[Peter Maydell]

On 12 October 2012 06:43, Andre Beckus <mikemail98-qemu@yahoo.com> wrote:
> 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?

That sounds like a good idea; we already handle the ID registers in
these functions because they're multi-width accessible.

-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/armv7m_nvic: Add global variable for SysTick external reference clock

[Peter Maydell]

On 12 October 2012 07:53, Andre Beckus <mikemail98-qemu@yahoo.com> wrote:
> As a case study, the STM32 does have a reference clock.  It is simply
> the system clock divided by 8 (maybe not ARM's intention for it to be
> tied so closely to the system clock).  The documentation says the TENMS
> field is hardwired to 9000, which corresponds to a 1 ms period when the
> external reference clock is selected and the system clock is running at
> 72 Mhz.  So, the TENMS field will not be accurate if the system clock is
> running at a different frequency (the SKEW bit is hardwired to 1).

OK, so the board needs to be able to separately set all of:
 * TENMS calibration field
 * system clock
 * reference clock

> Looking at the big picture, it seems that QEMU could benefit from a new
> "clock line" type for handling clock signals.  They could be exposed by
> devices in a similar manner to GPIO lines (there would be both input and
> output clock lines).  I could see them being useful (at least in the
> microcontroller world) for passing clock signals back and forth between
> peripherals, interfacing timer peripherals to machines, setting
> oscillator frequencies, and serving as the plumbing for clock trees.  I
> know I had to do a lot of hacking with the STM32 implementation to
> propagate the clock controller's signals to the other peripherals.
>
> When I searched on the topic, I saw that you discussed/requested a
> common clock framework back in July (in regards to the an exynos4210
> patch).  Do you know if any progress was made?

I haven't seen anything since then... I agree that a 'clock line'
connection might be useful.

-- PMM