hw/arm/netduino2, netduinoplus2: Set system_clock_scale

[PATCH for-5.1] hw/arm/netduino2, netduinoplus2: Set system_clock_scale

Posted by Peter Maydell 3 years, 9 months ago

The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale
global, which meant that if guest code used the systick timer in "use
the processor clock" mode it would hang because time never advances.

Set the global to match the documented CPU clock speed of these boards.
Judging by the data sheet this is slightly simplistic because the
SoC allows configuration of the SYSCLK source and frequency via the
RCC (reset and clock control) module, but we don't model that.

Fixes: https://bugs.launchpad.net/qemu/+bug/1876187
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
NB: tested with "make check" only...

 hw/arm/netduino2.c     | 10 ++++++++++
 hw/arm/netduinoplus2.c | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 79e19392b56..8f103341443 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -30,10 +30,20 @@
 #include "hw/arm/stm32f205_soc.h"
 #include "hw/arm/boot.h"
 
+/* Main SYSCLK frequency in Hz (120MHz) */
+#define SYSCLK_FRQ 120000000ULL
+
 static void netduino2_init(MachineState *machine)
 {
     DeviceState *dev;
 
+    /*
+     * TODO: ideally we would model the SoC RCC and let it handle
+     * system_clock_scale, including its ability to define different
+     * possible SYSCLK sources.
+     */
+    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
+
     dev = qdev_new(TYPE_STM32F205_SOC);
     qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
index 958d21dd9f9..68abd3ec69d 100644
--- a/hw/arm/netduinoplus2.c
+++ b/hw/arm/netduinoplus2.c
@@ -30,10 +30,20 @@
 #include "hw/arm/stm32f405_soc.h"
 #include "hw/arm/boot.h"
 
+/* Main SYSCLK frequency in Hz (168MHz) */
+#define SYSCLK_FRQ 168000000ULL
+
 static void netduinoplus2_init(MachineState *machine)
 {
     DeviceState *dev;
 
+    /*
+     * TODO: ideally we would model the SoC RCC and let it handle
+     * system_clock_scale, including its ability to define different
+     * possible SYSCLK sources.
+     */
+    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
+
     dev = qdev_new(TYPE_STM32F405_SOC);
     qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-- 
2.20.1

Re: [PATCH for-5.1] hw/arm/netduino2, netduinoplus2: Set system_clock_scale

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

On 7/27/20 6:26 PM, Peter Maydell wrote:
> The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale
> global, which meant that if guest code used the systick timer in "use
> the processor clock" mode it would hang because time never advances.
> 
> Set the global to match the documented CPU clock speed of these boards.
> Judging by the data sheet this is slightly simplistic because the
> SoC allows configuration of the SYSCLK source and frequency via the
> RCC (reset and clock control) module, but we don't model that.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1876187
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: tested with "make check" only...

What about adding another patch with:

-- >8 --
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -17,6 +17,7 @@
 #include "qemu/timer.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
 #include "trace.h"

 /* qemu timers run at 1GHz.   We want something closer to 1MHz.  */
@@ -221,6 +222,11 @@ static void systick_instance_init(Object *obj)
 static void systick_realize(DeviceState *dev, Error **errp)
 {
     SysTickState *s = SYSTICK(dev);
+
+    if (!system_clock_scale) {
+        error_setg(errp, "can not use systick with 'system_clock_scale
= 0'");
+        return;
+    }
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, systick_timer_tick, s);
 }

---

I'll try to review with the datasheets tomorrow morning in case you want
to get it merged during the day.

> 
>  hw/arm/netduino2.c     | 10 ++++++++++
>  hw/arm/netduinoplus2.c | 10 ++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index 79e19392b56..8f103341443 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -30,10 +30,20 @@
>  #include "hw/arm/stm32f205_soc.h"
>  #include "hw/arm/boot.h"
>  
> +/* Main SYSCLK frequency in Hz (120MHz) */
> +#define SYSCLK_FRQ 120000000ULL
> +
>  static void netduino2_init(MachineState *machine)
>  {
>      DeviceState *dev;
>  
> +    /*
> +     * TODO: ideally we would model the SoC RCC and let it handle
> +     * system_clock_scale, including its ability to define different
> +     * possible SYSCLK sources.
> +     */
> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
> +
>      dev = qdev_new(TYPE_STM32F205_SOC);
>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
> index 958d21dd9f9..68abd3ec69d 100644
> --- a/hw/arm/netduinoplus2.c
> +++ b/hw/arm/netduinoplus2.c
> @@ -30,10 +30,20 @@
>  #include "hw/arm/stm32f405_soc.h"
>  #include "hw/arm/boot.h"
>  
> +/* Main SYSCLK frequency in Hz (168MHz) */
> +#define SYSCLK_FRQ 168000000ULL
> +
>  static void netduinoplus2_init(MachineState *machine)
>  {
>      DeviceState *dev;
>  
> +    /*
> +     * TODO: ideally we would model the SoC RCC and let it handle
> +     * system_clock_scale, including its ability to define different
> +     * possible SYSCLK sources.
> +     */
> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
> +
>      dev = qdev_new(TYPE_STM32F405_SOC);
>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>

Re: [PATCH for-5.1] hw/arm/netduino2, netduinoplus2: Set system_clock_scale

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

On 7/27/20 8:05 PM, Philippe Mathieu-Daudé wrote:
> On 7/27/20 6:26 PM, Peter Maydell wrote:
>> The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale
>> global, which meant that if guest code used the systick timer in "use
>> the processor clock" mode it would hang because time never advances.
>>
>> Set the global to match the documented CPU clock speed of these boards.
>> Judging by the data sheet this is slightly simplistic because the
>> SoC allows configuration of the SYSCLK source and frequency via the
>> RCC (reset and clock control) module, but we don't model that.
>>
>> Fixes: https://bugs.launchpad.net/qemu/+bug/1876187
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> NB: tested with "make check" only...
> 
> What about adding another patch with:
> 
> -- >8 --
> --- a/hw/timer/armv7m_systick.c
> +++ b/hw/timer/armv7m_systick.c
> @@ -17,6 +17,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "qapi/error.h"
>  #include "trace.h"
> 
>  /* qemu timers run at 1GHz.   We want something closer to 1MHz.  */
> @@ -221,6 +222,11 @@ static void systick_instance_init(Object *obj)
>  static void systick_realize(DeviceState *dev, Error **errp)
>  {
>      SysTickState *s = SYSTICK(dev);
> +
> +    if (!system_clock_scale) {
> +        error_setg(errp, "can not use systick with 'system_clock_scale
> = 0'");
> +        return;
> +    }
>      s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, systick_timer_tick, s);
>  }

Hmm testing the microbit:

$ qemu-system-arm -M microbit
qemu-system-arm: can not use systick with 'system_clock_scale = 0'

> 
> ---
> 
> I'll try to review with the datasheets tomorrow morning in case you want
> to get it merged during the day.
> 
>>
>>  hw/arm/netduino2.c     | 10 ++++++++++
>>  hw/arm/netduinoplus2.c | 10 ++++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>> index 79e19392b56..8f103341443 100644
>> --- a/hw/arm/netduino2.c
>> +++ b/hw/arm/netduino2.c
>> @@ -30,10 +30,20 @@
>>  #include "hw/arm/stm32f205_soc.h"
>>  #include "hw/arm/boot.h"
>>  
>> +/* Main SYSCLK frequency in Hz (120MHz) */
>> +#define SYSCLK_FRQ 120000000ULL
>> +
>>  static void netduino2_init(MachineState *machine)
>>  {
>>      DeviceState *dev;
>>  
>> +    /*
>> +     * TODO: ideally we would model the SoC RCC and let it handle
>> +     * system_clock_scale, including its ability to define different
>> +     * possible SYSCLK sources.
>> +     */
>> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
>> +
>>      dev = qdev_new(TYPE_STM32F205_SOC);
>>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
>> index 958d21dd9f9..68abd3ec69d 100644
>> --- a/hw/arm/netduinoplus2.c
>> +++ b/hw/arm/netduinoplus2.c
>> @@ -30,10 +30,20 @@
>>  #include "hw/arm/stm32f405_soc.h"
>>  #include "hw/arm/boot.h"
>>  
>> +/* Main SYSCLK frequency in Hz (168MHz) */
>> +#define SYSCLK_FRQ 168000000ULL
>> +
>>  static void netduinoplus2_init(MachineState *machine)
>>  {
>>      DeviceState *dev;
>>  
>> +    /*
>> +     * TODO: ideally we would model the SoC RCC and let it handle
>> +     * system_clock_scale, including its ability to define different
>> +     * possible SYSCLK sources.
>> +     */
>> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
>> +
>>      dev = qdev_new(TYPE_STM32F405_SOC);
>>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
>>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>
> 
>

Re: [PATCH for-5.1] hw/arm/netduino2, netduinoplus2: Set system_clock_scale

Posted by Peter Maydell 3 years, 9 months ago

On Mon, 27 Jul 2020 at 19:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> $ qemu-system-arm -M microbit
> qemu-system-arm: can not use systick with 'system_clock_scale = 0'

Yes, that's the other one on my list to do next (need to find
out what frequency it runs at though).

Good idea to have an automatic check for system_clock_scale
being set, but failure to do that is a bug in the board
code, so we might as well assert() it.

-- PMM

Re: [PATCH for-5.1] hw/arm/netduino2, netduinoplus2: Set system_clock_scale

Posted by Alistair Francis 3 years, 9 months ago

On Mon, Jul 27, 2020 at 9:26 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The netduino2 and netduinoplus2 boards forgot to set the system_clock_scale
> global, which meant that if guest code used the systick timer in "use
> the processor clock" mode it would hang because time never advances.
>
> Set the global to match the documented CPU clock speed of these boards.
> Judging by the data sheet this is slightly simplistic because the
> SoC allows configuration of the SYSCLK source and frequency via the
> RCC (reset and clock control) module, but we don't model that.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1876187
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> NB: tested with "make check" only...
>
>  hw/arm/netduino2.c     | 10 ++++++++++
>  hw/arm/netduinoplus2.c | 10 ++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index 79e19392b56..8f103341443 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -30,10 +30,20 @@
>  #include "hw/arm/stm32f205_soc.h"
>  #include "hw/arm/boot.h"
>
> +/* Main SYSCLK frequency in Hz (120MHz) */
> +#define SYSCLK_FRQ 120000000ULL
> +
>  static void netduino2_init(MachineState *machine)
>  {
>      DeviceState *dev;
>
> +    /*
> +     * TODO: ideally we would model the SoC RCC and let it handle
> +     * system_clock_scale, including its ability to define different
> +     * possible SYSCLK sources.
> +     */
> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
> +
>      dev = qdev_new(TYPE_STM32F205_SOC);
>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
> index 958d21dd9f9..68abd3ec69d 100644
> --- a/hw/arm/netduinoplus2.c
> +++ b/hw/arm/netduinoplus2.c
> @@ -30,10 +30,20 @@
>  #include "hw/arm/stm32f405_soc.h"
>  #include "hw/arm/boot.h"
>
> +/* Main SYSCLK frequency in Hz (168MHz) */
> +#define SYSCLK_FRQ 168000000ULL
> +
>  static void netduinoplus2_init(MachineState *machine)
>  {
>      DeviceState *dev;
>
> +    /*
> +     * TODO: ideally we would model the SoC RCC and let it handle
> +     * system_clock_scale, including its ability to define different
> +     * possible SYSCLK sources.
> +     */
> +    system_clock_scale = NANOSECONDS_PER_SECOND / SYSCLK_FRQ;
> +
>      dev = qdev_new(TYPE_STM32F405_SOC);
>      qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> --
> 2.20.1
>
>