Patchwork [2/2] hw/armv7m_nvic: Add global variable for SysTick external reference clock

login
register
mail settings
Submitter Andre Beckus
Date Oct. 9, 2012, 10:29 p.m.
Message ID <1349821756-23125-2-git-send-email-mikemail98-qemu@yahoo.com>
Download mbox | patch
Permalink /patch/190589/
State New
Headers show

Comments

Andre Beckus - Oct. 9, 2012, 10:29 p.m.
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(-)
Peter Maydell - Oct. 10, 2012, 12:03 p.m.
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
Andre Beckus - Oct. 12, 2012, 6:53 a.m.
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
Peter Maydell - Oct. 12, 2012, 8:36 a.m.
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

Patch

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)