| Message ID | 1349821756-23125-2-git-send-email-mikemail98-qemu@yahoo.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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)
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(-)