[PATCH] hw/arm/armv7m: Handle disconnected clock inputs

[PATCH] hw/arm/armv7m: Handle disconnected clock inputs

[Peter Maydell]

In the armv7m object, handle clock inputs that aren't connected.
This is always an error for 'cpuclk'. For 'refclk' it is OK for this
to be disconnected, but we need to handle it by not trying to connect
a sourceless-clock to the systick device.

This fixes a bug where on the mps2-an521 and similar boards (which
do not have a refclk) the systick device incorrectly reset with
SYST_CSR.CLKSOURCE 0 ("use refclk") rather than 1 ("use CPU clock").

Cc: qemu-stable@nongnu.org
Reported-by: Richard Petri <git@rpls.de>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The other option would be to have clock_has_source() look not
just at clk->source but somehow walk up the clock tree to see
if it can find something that looks like a "root". That seems
overcomplicated...
---
 hw/arm/armv7m.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index ceb76df3cd4..41cfca0f223 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -284,6 +284,12 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* cpuclk must be connected; refclk is optional */
+    if (!clock_has_source(s->cpuclk)) {
+        error_setg(errp, "armv7m: cpuclk must be connected");
+        return;
+    }
+
     memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
 
     s->cpu = ARM_CPU(object_new_with_props(s->cpu_type, OBJECT(s), "cpu",
@@ -420,8 +426,18 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
                                     &s->sysreg_ns_mem);
     }
 
-    /* Create and map the systick devices */
-    qdev_connect_clock_in(DEVICE(&s->systick[M_REG_NS]), "refclk", s->refclk);
+    /*
+     * Create and map the systick devices. Note that we only connect
+     * refclk if it has been connected to us; otherwise the systick
+     * device gets the wrong answer for clock_has_source(refclk), because
+     * it has an immediate source (the ARMv7M's clock object) but not
+     * an ultimate source, and then it won't correctly auto-select the
+     * CPU clock as its only possible clock source.
+     */
+    if (clock_has_source(s->refclk)) {
+        qdev_connect_clock_in(DEVICE(&s->systick[M_REG_NS]), "refclk",
+                              s->refclk);
+    }
     qdev_connect_clock_in(DEVICE(&s->systick[M_REG_NS]), "cpuclk", s->cpuclk);
     if (!sysbus_realize(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), errp)) {
         return;
@@ -438,8 +454,10 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
          */
         object_initialize_child(OBJECT(dev), "systick-reg-s",
                                 &s->systick[M_REG_S], TYPE_SYSTICK);
-        qdev_connect_clock_in(DEVICE(&s->systick[M_REG_S]), "refclk",
-                              s->refclk);
+        if (clock_has_source(s->refclk)) {
+            qdev_connect_clock_in(DEVICE(&s->systick[M_REG_S]), "refclk",
+                                  s->refclk);
+        }
         qdev_connect_clock_in(DEVICE(&s->systick[M_REG_S]), "cpuclk",
                               s->cpuclk);
 
-- 
2.25.1

Re: [PATCH] hw/arm/armv7m: Handle disconnected clock inputs

[Philippe Mathieu-Daudé via]

On 8/2/22 18:16, Peter Maydell wrote:
> In the armv7m object, handle clock inputs that aren't connected.
> This is always an error for 'cpuclk'. For 'refclk' it is OK for this
> to be disconnected, but we need to handle it by not trying to connect
> a sourceless-clock to the systick device.
> 
> This fixes a bug where on the mps2-an521 and similar boards (which
> do not have a refclk) the systick device incorrectly reset with
> SYST_CSR.CLKSOURCE 0 ("use refclk") rather than 1 ("use CPU clock").
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Richard Petri <git@rpls.de>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The other option would be to have clock_has_source() look not
> just at clk->source but somehow walk up the clock tree to see
> if it can find something that looks like a "root". That seems
> overcomplicated...
> ---
>   hw/arm/armv7m.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH] hw/arm/armv7m: Handle disconnected clock inputs

[Richard Henderson]

On 2/9/22 04:16, Peter Maydell wrote:
> In the armv7m object, handle clock inputs that aren't connected.
> This is always an error for 'cpuclk'. For 'refclk' it is OK for this
> to be disconnected, but we need to handle it by not trying to connect
> a sourceless-clock to the systick device.
> 
> This fixes a bug where on the mps2-an521 and similar boards (which
> do not have a refclk) the systick device incorrectly reset with
> SYST_CSR.CLKSOURCE 0 ("use refclk") rather than 1 ("use CPU clock").
> 
> Cc:qemu-stable@nongnu.org
> Reported-by: Richard Petri<git@rpls.de>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> The other option would be to have clock_has_source() look not
> just at clk->source but somehow walk up the clock tree to see
> if it can find something that looks like a "root". That seems
> overcomplicated...
> ---
>   hw/arm/armv7m.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~