qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
@ 2021-04-09  6:23 Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 1/9] hw/core/clock: Increase clock propagation trace events verbosity Philippe Mathieu-Daudé
                   ` (10 more replies)
  0 siblings, 11 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini

Hi Damian, Luc, Peter.

I've been debugging some odd issue with the clocks:
a clock created in the machine (IOW, not a qdev clock) isn't
always resetted, thus propagating its value.
"not always" is the odd part. In the MPS2 board, the machine
clock is propagated. Apparently because the peripherals are
created directly in the machine_init() handler. When moving
them out in a SoC QOM container, the clock isn't... I'm still
having hard time to understand what is going on.

Alternatively I tried to strengthen the clock API by reducing
the clock creation in 2 cases: machine/device. This way clocks
aren't left dangling around alone. The qdev clocks are properly
resetted, and for the machine clocks I register a generic reset
handler. This way is safer, but I don't think we want to keep
adding generic reset handlers, instead we'd like to remove them.

I'll keep debugging to understand. Meanwhile posting this series
as RFC to get feedback on the approach and start discussing on
this issue.

Regards,

Phil.

Philippe Mathieu-Daudé (9):
  hw/core/clock: Increase clock propagation trace events verbosity
  hw/core/machine: Add machine_create_constant_clock() helper
  hw/arm: Use new machine_create_constant_clock() helper
  hw/mips: Use new machine_create_constant_clock() helper
  hw/core/qdev-clock: Add qdev_ground_clock() helper
  hw/misc/bcm2835_cprman: Use qdev_ground_clock() helper
  hw/misc/bcm2835_cprman: Feed 'xosc' from the board
  hw/clock: Declare clock_new() internally
  hw/core/machine: Reset machine clocks using qemu_register_reset()

 hw/core/clock-internal.h         | 32 ++++++++++++++++++++++++++++++++
 include/hw/boards.h              | 17 +++++++++++++++++
 include/hw/clock.h               | 13 -------------
 include/hw/misc/bcm2835_cprman.h |  2 --
 include/hw/qdev-clock.h          |  9 +++++++++
 hw/arm/bcm2835_peripherals.c     |  1 +
 hw/arm/bcm2836.c                 |  1 +
 hw/arm/mps2-tz.c                 |  6 ++----
 hw/arm/mps2.c                    |  3 +--
 hw/arm/musca.c                   |  6 ++----
 hw/arm/raspi.c                   |  4 ++++
 hw/core/clock.c                  |  9 ++++++++-
 hw/core/machine.c                | 20 ++++++++++++++++++++
 hw/core/qdev-clock.c             | 12 ++++++++++++
 hw/mips/fuloong2e.c              |  4 ++--
 hw/mips/jazz.c                   |  6 +++---
 hw/mips/loongson3_virt.c         |  4 ++--
 hw/mips/mipssim.c                |  7 ++++---
 hw/misc/bcm2835_cprman.c         | 12 +++---------
 MAINTAINERS                      |  1 +
 hw/core/trace-events             |  3 ++-
 21 files changed, 126 insertions(+), 46 deletions(-)
 create mode 100644 hw/core/clock-internal.h

-- 
2.26.3



^ permalink raw reply	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 1/9] hw/core/clock: Increase clock propagation trace events verbosity
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 2/9] hw/core/machine: Add machine_create_constant_clock() helper Philippe Mathieu-Daudé
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini

Move the 'clock_propagate' trace event to clock_propagate_period()
to display the recursive propagation, and add 'propagate_children'
event to closely look at the clock propagation for each children
in the clock tree.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/clock.c      | 8 +++++++-
 hw/core/trace-events | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/core/clock.c b/hw/core/clock.c
index fc5a99683f8..a42dc3c3d29 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -79,7 +79,14 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks)
 {
     Clock *child;
 
+    trace_clock_propagate(CLOCK_PATH(clk),
+                          CLOCK_PERIOD_TO_HZ(clk->period),
+                          call_callbacks);
     QLIST_FOREACH(child, &clk->children, sibling) {
+        trace_clock_propagate_children(CLOCK_PATH(clk),
+                                       CLOCK_PERIOD_TO_HZ(clk->period),
+                                       CLOCK_PATH(child),
+                                       CLOCK_PERIOD_TO_HZ(child->period));
         if (child->period != clk->period) {
             if (call_callbacks) {
                 clock_call_callback(child, ClockPreUpdate);
@@ -99,7 +106,6 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks)
 void clock_propagate(Clock *clk)
 {
     assert(clk->source == NULL);
-    trace_clock_propagate(CLOCK_PATH(clk));
     clock_propagate_period(clk, true);
 }
 
diff --git a/hw/core/trace-events b/hw/core/trace-events
index 360ddeb2c87..22df6789918 100644
--- a/hw/core/trace-events
+++ b/hw/core/trace-events
@@ -32,5 +32,6 @@ resettable_transitional_function(void *obj, const char *objtype) "obj=%p(%s)"
 clock_set_source(const char *clk, const char *src) "'%s', src='%s'"
 clock_disconnect(const char *clk) "'%s'"
 clock_set(const char *clk, uint64_t old, uint64_t new) "'%s', %"PRIu64"Hz->%"PRIu64"Hz"
-clock_propagate(const char *clk) "'%s'"
+clock_propagate(const char *src, uint64_t src_hz, int cb) "src='%s' val=%"PRIu64"Hz cb=%d"
+clock_propagate_children(const char *src, uint64_t src_hz, const char *clk, uint64_t clk_hz) "src='%s'@%"PRIu64"Hz -> clk='%s'@%"PRIu64"Hz"
 clock_update(const char *clk, const char *src, uint64_t hz, int cb) "'%s', src='%s', val=%"PRIu64"Hz cb=%d"
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 2/9] hw/core/machine: Add machine_create_constant_clock() helper
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 1/9] hw/core/clock: Increase clock propagation trace events verbosity Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 3/9] hw/arm: Use new " Philippe Mathieu-Daudé
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini

Boards usually have crystal oscillators (at fixed frequency)
feeding their various clocked components.

Add a helper to create such constant clocks in the machine.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/boards.h | 17 +++++++++++++++++
 hw/core/machine.c   | 11 +++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index ad6c8fd5376..c8100f7a33a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -11,6 +11,7 @@
 #include "qemu/module.h"
 #include "qom/object.h"
 #include "hw/core/cpu.h"
+#include "hw/clock.h"
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 
@@ -83,6 +84,22 @@ bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev);
 MemoryRegion *machine_consume_memdev(MachineState *machine,
                                      HostMemoryBackend *backend);
 
+/**
+ * machine_create_constant_clock:
+ * @machine: the parent machine
+ * @name: the clock object name
+ * @freq_hz: the clock frequency (in Hz)
+ *
+ * Helper function to create a new constant clock (fixed frequency
+ * of @freq_hz) and parent it to @machine. There is no need to call
+ * clock_setup_canonical_path on the returned clock as it is done
+ * by this function.
+ *
+ * @return the newly created clock
+ */
+Clock *machine_create_constant_clock(MachineState *machine,
+                                     const char *name, unsigned freq_hz);
+
 /**
  * CPUArchId:
  * @arch_id - architecture-dependent CPU ID of present or possible CPU
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 40def78183a..41baf80559d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1233,6 +1233,17 @@ void machine_run_board_init(MachineState *machine)
     phase_advance(PHASE_MACHINE_INITIALIZED);
 }
 
+Clock *machine_create_constant_clock(MachineState *machine,
+                                     const char *name, unsigned freq_hz)
+{
+    Clock *clk;
+
+    clk = clock_new(OBJECT(machine), name);
+    clock_set_hz(clk, freq_hz);
+
+    return clk;
+}
+
 static NotifierList machine_init_done_notifiers =
     NOTIFIER_LIST_INITIALIZER(machine_init_done_notifiers);
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 3/9] hw/arm: Use new machine_create_constant_clock() helper
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 1/9] hw/core/clock: Increase clock propagation trace events verbosity Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 2/9] hw/core/machine: Add machine_create_constant_clock() helper Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 4/9] hw/mips: " Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini

Use the newly added machine_create_constant_clock() helper
to create the SYSCLKs.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/mps2-tz.c | 6 ++----
 hw/arm/mps2.c    | 3 +--
 hw/arm/musca.c   | 6 ++----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 3fbe3d29f95..86481fbe40d 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -740,10 +740,8 @@ static void mps2tz_common_init(MachineState *machine)
     }
 
     /* These clocks don't need migration because they are fixed-frequency */
-    mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, mmc->sysclk_frq);
-    mms->s32kclk = clock_new(OBJECT(machine), "S32KCLK");
-    clock_set_hz(mms->s32kclk, S32KCLK_FRQ);
+    mms->sysclk = machine_create_constant_clock(machine, "SYSCLK", mmc->sysclk_frq);
+    mms->s32kclk = machine_create_constant_clock(machine, "S32KCLK", S32KCLK_FRQ);
 
     object_initialize_child(OBJECT(machine), TYPE_IOTKIT, &mms->iotkit,
                             mmc->armsse_type);
diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 81413b7133e..5d9250da615 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -143,8 +143,7 @@ static void mps2_common_init(MachineState *machine)
     }
 
     /* This clock doesn't need migration because it is fixed-frequency */
-    mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, SYSCLK_FRQ);
+    mms->sysclk = machine_create_constant_clock(machine, "SYSCLK", SYSCLK_FRQ);
 
     /* The FPGA images have an odd combination of different RAMs,
      * because in hardware they are different implementations and
diff --git a/hw/arm/musca.c b/hw/arm/musca.c
index 7a83f7dda7d..e15149f3a27 100644
--- a/hw/arm/musca.c
+++ b/hw/arm/musca.c
@@ -372,10 +372,8 @@ static void musca_init(MachineState *machine)
         exit(1);
     }
 
-    mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, SYSCLK_FRQ);
-    mms->s32kclk = clock_new(OBJECT(machine), "S32KCLK");
-    clock_set_hz(mms->s32kclk, S32KCLK_FRQ);
+    mms->sysclk = machine_create_constant_clock(machine, "SYSCLK", SYSCLK_FRQ);
+    mms->s32kclk = machine_create_constant_clock(machine, "S32KCLK", S32KCLK_FRQ);
 
     object_initialize_child(OBJECT(machine), "sse-200", &mms->sse,
                             TYPE_SSE200);
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 4/9] hw/mips: Use new machine_create_constant_clock() helper
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 3/9] hw/arm: Use new " Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Aleksandar Rikalo, Luc Michel,
	Eduardo Habkost, Huacai Chen, Philippe Mathieu-Daudé,
	qemu-arm, Hervé Poussineau, Paolo Bonzini, Aurelien Jarno

Use the newly added machine_create_constant_clock() helper
to create the CPU reference clocks.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/fuloong2e.c      | 4 ++--
 hw/mips/jazz.c           | 6 +++---
 hw/mips/loongson3_virt.c | 4 ++--
 hw/mips/mipssim.c        | 7 ++++---
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 4f61f2c873b..72dc5702727 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -259,8 +259,8 @@ static void mips_fuloong2e_init(MachineState *machine)
     CPUMIPSState *env;
     DeviceState *dev;
 
-    cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
-    clock_set_hz(cpuclk, 533080000); /* ~533 MHz */
+    cpuclk = machine_create_constant_clock(machine, "cpu-refclk",
+                                           533080000); /* ~533 MHz */
 
     /* init CPUs */
     cpu = mips_cpu_create_with_clock(machine->cpu_type, cpuclk);
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 1a0888a0fd5..3c220b4df6b 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -185,9 +185,9 @@ static void mips_jazz_init(MachineState *machine,
         exit(EXIT_FAILURE);
     }
 
-    cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
-    clock_set_hz(cpuclk, ext_clk[jazz_model].freq_hz
-                         * ext_clk[jazz_model].pll_mult);
+    cpuclk = machine_create_constant_clock(machine, "cpu-refclk",
+                                           ext_clk[jazz_model].freq_hz
+                                           * ext_clk[jazz_model].pll_mult);
 
     /* init CPUs */
     cpu = mips_cpu_create_with_clock(machine->cpu_type, cpuclk);
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index b15071defc6..b9f18ecd911 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -530,8 +530,8 @@ static void mips_loongson3_virt_init(MachineState *machine)
     sysbus_create_simple("goldfish_rtc", virt_memmap[VIRT_RTC].base,
                          qdev_get_gpio_in(liointc, RTC_IRQ));
 
-    cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
-    clock_set_hz(cpuclk, DEF_LOONGSON3_FREQ);
+    cpuclk = machine_create_constant_clock(machine, "cpu-refclk",
+                                           DEF_LOONGSON3_FREQ);
 
     for (i = 0; i < machine->smp.cpus; i++) {
         int ip;
diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
index f5d0da05aa1..af11cf9ac25 100644
--- a/hw/mips/mipssim.c
+++ b/hw/mips/mipssim.c
@@ -153,11 +153,12 @@ mips_mipssim_init(MachineState *machine)
     ResetData *reset_info;
     int bios_size;
 
-    cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
 #ifdef TARGET_MIPS64
-    clock_set_hz(cpuclk, 6000000); /* 6 MHz */
+    cpuclk = machine_create_constant_clock(machine, "cpu-refclk",
+                                           6000000); /* 6 MHz */
 #else
-    clock_set_hz(cpuclk, 12000000); /* 12 MHz */
+    cpuclk = machine_create_constant_clock(machine, "cpu-refclk",
+                                           12000000); /* 12 MHz */
 #endif
 
     /* Init CPUs. */
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 4/9] hw/mips: " Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-19 14:22   ` Peter Maydell
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 6/9] hw/misc/bcm2835_cprman: Use " Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé, qemu-arm, Daniel P. Berrangé,
	Paolo Bonzini

Clocks are rarely left unconnected, but rather connected to ground
plane to avoid noise. When representing the clock tree, we want to
see such ground clock. As we might reuse this clock on various
boards, introduce the qdev_ground_clock() which return a singleton
ground clock.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/qdev-clock.h |  9 +++++++++
 hw/core/qdev-clock.c    | 11 +++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
index ffa0f7ba09e..2f2d2da6cd6 100644
--- a/include/hw/qdev-clock.h
+++ b/include/hw/qdev-clock.h
@@ -161,4 +161,13 @@ typedef struct ClockPortInitElem ClockPortInitArray[];
  */
 void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
 
+/**
+ * qdev_ground_clock:
+ * @returns: a pointer to the ground clock
+ *
+ * Get the special 'ground' clock. This clock can be used as input
+ * (unclocked) or output.
+ */
+Clock *qdev_ground_clock(void);
+
 #endif /* QDEV_CLOCK_H */
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index 117f4c6ea4a..a46384a84b7 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -210,3 +210,14 @@ void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
     assert(!dev->realized);
     clock_set_source(qdev_get_clock_in(dev, name), source);
 }
+
+Clock *qdev_ground_clock(void)
+{
+    static Clock *gnd_clk;
+
+    if (!gnd_clk) {
+        gnd_clk = clock_new(qdev_get_machine(), "gnd");
+    }
+
+    return gnd_clk;
+}
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 6/9] hw/misc/bcm2835_cprman: Use qdev_ground_clock() helper
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	Andrew Baumann, qemu-arm, Paolo Bonzini

The ground clock isn't really an internal component of the CPRMAN
peripheral. Use the qdev_ground_clock() helper to access the board
ground plane.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/bcm2835_cprman.h | 1 -
 hw/misc/bcm2835_cprman.c         | 7 ++-----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/hw/misc/bcm2835_cprman.h b/include/hw/misc/bcm2835_cprman.h
index 3df4ceedd2e..2996ccb4632 100644
--- a/include/hw/misc/bcm2835_cprman.h
+++ b/include/hw/misc/bcm2835_cprman.h
@@ -204,7 +204,6 @@ struct BCM2835CprmanState {
     uint32_t xosc_freq;
 
     Clock *xosc;
-    Clock *gnd;
 };
 
 #endif
diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
index 75e6c574d46..5039b7632b4 100644
--- a/hw/misc/bcm2835_cprman.c
+++ b/hw/misc/bcm2835_cprman.c
@@ -678,9 +678,6 @@ static void cprman_init(Object *obj)
     }
 
     s->xosc = clock_new(obj, "xosc");
-    s->gnd = clock_new(obj, "gnd");
-
-    clock_set(s->gnd, 0);
 
     memory_region_init_io(&s->iomem, obj, &cprman_ops,
                           s, "bcm2835-cprman", 0x2000);
@@ -697,7 +694,7 @@ static void connect_mux_sources(BCM2835CprmanState *s,
 
     /* For sources from 0 to 3. Source 4 to 9 are mux specific */
     Clock * const CLK_SRC_MAPPING[] = {
-        [CPRMAN_CLOCK_SRC_GND] = s->gnd,
+        [CPRMAN_CLOCK_SRC_GND] = qdev_ground_clock(),
         [CPRMAN_CLOCK_SRC_XOSC] = s->xosc,
         [CPRMAN_CLOCK_SRC_TD0] = td0,
         [CPRMAN_CLOCK_SRC_TD1] = td1,
@@ -708,7 +705,7 @@ static void connect_mux_sources(BCM2835CprmanState *s,
         Clock *src;
 
         if (mapping == CPRMAN_CLOCK_SRC_FORCE_GROUND) {
-            src = s->gnd;
+            src = qdev_ground_clock();
         } else if (mapping == CPRMAN_CLOCK_SRC_DSI0HSCK) {
             src = s->dsi0hsck_mux.out;
         } else if (i < CPRMAN_CLOCK_SRC_PLLA) {
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 6/9] hw/misc/bcm2835_cprman: Use " Philippe Mathieu-Daudé
@ 2021-04-09  6:23 ` Philippe Mathieu-Daudé
  2021-04-19 14:24   ` Peter Maydell
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	Andrew Baumann, qemu-arm, Paolo Bonzini

The XOsc signal isn't a part of the CPRMAN, but comes from a
crystal oscillator external to the SoC.

Create the oscillator on the board, and propagate it to the
CPRMAN.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/bcm2835_cprman.h | 1 -
 hw/arm/bcm2835_peripherals.c     | 1 +
 hw/arm/bcm2836.c                 | 1 +
 hw/arm/raspi.c                   | 4 ++++
 hw/misc/bcm2835_cprman.c         | 5 +----
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/hw/misc/bcm2835_cprman.h b/include/hw/misc/bcm2835_cprman.h
index 2996ccb4632..52f76489f36 100644
--- a/include/hw/misc/bcm2835_cprman.h
+++ b/include/hw/misc/bcm2835_cprman.h
@@ -201,7 +201,6 @@ struct BCM2835CprmanState {
     CprmanDsi0HsckMuxState dsi0hsck_mux;
 
     uint32_t regs[CPRMAN_NUM_REGS];
-    uint32_t xosc_freq;
 
     Clock *xosc;
 };
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index dcff13433e5..a82f2b42f5a 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -123,6 +123,7 @@ static void bcm2835_peripherals_init(Object *obj)
 
     /* CPRMAN clock manager */
     object_initialize_child(obj, "cprman", &s->cprman, TYPE_BCM2835_CPRMAN);
+    qdev_alias_clock(DEVICE(&s->cprman), "xosc-in", DEVICE(s), "xosc-in");
 
     object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
                                    OBJECT(&s->gpu_bus_mr));
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index de7ade2878e..6c238ecb949 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -63,6 +63,7 @@ static void bcm2836_init(Object *obj)
                               "board-rev");
     object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
                               "vcram-size");
+    qdev_alias_clock(DEVICE(&s->peripherals), "xosc-in", DEVICE(s), "xosc-in");
 }
 
 static bool bcm283x_common_realize(DeviceState *dev, Error **errp)
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 990509d3852..a89f7e17c3a 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -40,6 +40,7 @@ struct RaspiMachineState {
     /*< private >*/
     MachineState parent_obj;
     /*< public >*/
+    Clock *xosc;
     BCM283XState soc;
     struct arm_boot_info binfo;
 };
@@ -277,12 +278,15 @@ static void raspi_machine_init(MachineState *machine)
     memory_region_add_subregion_overlap(get_system_memory(), 0,
                                         machine->ram, 0);
 
+    s->xosc = machine_create_constant_clock(machine, "xosc", 19200000);
+
     /* Setup the SOC */
     object_initialize_child(OBJECT(machine), "soc", &s->soc,
                             board_soc_type(board_rev));
     object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(machine->ram));
     object_property_set_int(OBJECT(&s->soc), "board-rev", board_rev,
                             &error_abort);
+    qdev_connect_clock_in(DEVICE(&s->soc), "xosc-in", s->xosc);
     qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
 
     /* Create and plug in the SD cards */
diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
index 5039b7632b4..33f15d99f63 100644
--- a/hw/misc/bcm2835_cprman.c
+++ b/hw/misc/bcm2835_cprman.c
@@ -637,8 +637,6 @@ static void cprman_reset(DeviceState *dev)
     for (i = 0; i < CPRMAN_NUM_CLOCK_MUX; i++) {
         device_cold_reset(DEVICE(&s->clock_muxes[i]));
     }
-
-    clock_update_hz(s->xosc, s->xosc_freq);
 }
 
 static void cprman_init(Object *obj)
@@ -677,7 +675,7 @@ static void cprman_init(Object *obj)
         g_free(alias);
     }
 
-    s->xosc = clock_new(obj, "xosc");
+    s->xosc = qdev_init_clock_in(DEVICE(obj), "xosc-in", NULL, s, ClockUpdate);
 
     memory_region_init_io(&s->iomem, obj, &cprman_ops,
                           s, "bcm2835-cprman", 0x2000);
@@ -776,7 +774,6 @@ static const VMStateDescription cprman_vmstate = {
 };
 
 static Property cprman_properties[] = {
-    DEFINE_PROP_UINT32("xosc-freq-hz", BCM2835CprmanState, xosc_freq, 19200000),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board Philippe Mathieu-Daudé
@ 2021-04-09  6:24 ` Philippe Mathieu-Daudé
  2021-04-19 14:26   ` Peter Maydell
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset() Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  10 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé, qemu-arm, Daniel P. Berrangé,
	Paolo Bonzini

To enforce correct API usage, restrict the clock creation to
hw/core/. The only possible ways to create a clock are:

- Constant clock at the board level
  Using machine_create_constant_clock() in machine_init()

- Propagated clock in QDev
  Using qdev_init_clock_in() or qdev_init_clock_out() in
  TYPE_DEVICE instance_init().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/clock-internal.h | 32 ++++++++++++++++++++++++++++++++
 include/hw/clock.h       | 13 -------------
 hw/core/clock.c          |  1 +
 hw/core/machine.c        |  1 +
 hw/core/qdev-clock.c     |  1 +
 MAINTAINERS              |  1 +
 6 files changed, 36 insertions(+), 13 deletions(-)
 create mode 100644 hw/core/clock-internal.h

diff --git a/hw/core/clock-internal.h b/hw/core/clock-internal.h
new file mode 100644
index 00000000000..2207be74c0f
--- /dev/null
+++ b/hw/core/clock-internal.h
@@ -0,0 +1,32 @@
+/*
+ * Hardware Clocks
+ *
+ * Copyright GreenSocs 2016-2020
+ *
+ * Authors:
+ *  Frederic Konrad
+ *  Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HW_CLOCK_INTERNAL_H
+#define QEMU_HW_CLOCK_INTERNAL_H
+
+#include "hw/clock.h"
+
+/**
+ * clock_new:
+ * @parent: the clock parent
+ * @name: the clock object name
+ *
+ * Helper function to create a new clock and parent it to @parent. There is no
+ * need to call clock_setup_canonical_path on the returned clock as it is done
+ * by this function.
+ *
+ * @return the newly created clock
+ */
+Clock *clock_new(Object *parent, const char *name);
+
+#endif
diff --git a/include/hw/clock.h b/include/hw/clock.h
index a7187eab95e..47cb65edb32 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -109,19 +109,6 @@ extern const VMStateDescription vmstate_clock;
  */
 void clock_setup_canonical_path(Clock *clk);
 
-/**
- * clock_new:
- * @parent: the clock parent
- * @name: the clock object name
- *
- * Helper function to create a new clock and parent it to @parent. There is no
- * need to call clock_setup_canonical_path on the returned clock as it is done
- * by this function.
- *
- * @return the newly created clock
- */
-Clock *clock_new(Object *parent, const char *name);
-
 /**
  * clock_set_callback:
  * @clk: the clock to register the callback into
diff --git a/hw/core/clock.c b/hw/core/clock.c
index a42dc3c3d29..bfa54ca0a93 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "hw/clock.h"
+#include "clock-internal.h"
 #include "trace.h"
 
 #define CLOCK_PATH(_clk) (_clk->canonical_path)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 41baf80559d..e8bdcd10854 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@
 #include "exec/confidential-guest-support.h"
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/virtio-pci.h"
+#include "clock-internal.h"
 
 GlobalProperty hw_compat_5_2[] = {
     { "ICH9-LPC", "smm-compat", "on"},
diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
index a46384a84b7..09e14009fcd 100644
--- a/hw/core/qdev-clock.c
+++ b/hw/core/qdev-clock.c
@@ -16,6 +16,7 @@
 #include "hw/qdev-clock.h"
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
+#include "clock-internal.h"
 
 /*
  * qdev_init_clocklist:
diff --git a/MAINTAINERS b/MAINTAINERS
index 58f342108e9..2b10744169c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2925,6 +2925,7 @@ S: Maintained
 F: include/hw/clock.h
 F: include/hw/qdev-clock.h
 F: hw/core/clock.c
+F: hw/core/clock-internal.h
 F: hw/core/clock-vmstate.c
 F: hw/core/qdev-clock.c
 F: docs/devel/clocks.rst
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset()
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally Philippe Mathieu-Daudé
@ 2021-04-09  6:24 ` Philippe Mathieu-Daudé
  2021-04-19 14:27   ` Peter Maydell
  2021-04-09 13:12 ` [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
  2021-04-10 13:19 ` Luc Michel
  10 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09  6:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	Philippe Mathieu-Daudé,
	qemu-arm, Paolo Bonzini

While the documentation mentions:

  Note that if you are creating a clock with a fixed period which
  will never change (for example the main clock source of a board),
  then you'll have nothing else to do. This value will be propagated
  to other clocks when connecting the clocks together and devices
  will fetch the right value during the first reset.

the clocks created in machine_init() aren't propagating their value
because they are never reset (not part of the reset tree, such
TYPE_DEVICE).

Register a generic reset handler to have them properly reset.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/machine.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e8bdcd10854..2817fe6a567 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1234,6 +1234,13 @@ void machine_run_board_init(MachineState *machine)
     phase_advance(PHASE_MACHINE_INITIALIZED);
 }
 
+static void constant_clock_reset(void *opaque)
+{
+    Clock *clk = opaque;
+
+    clock_propagate(clk);
+}
+
 Clock *machine_create_constant_clock(MachineState *machine,
                                      const char *name, unsigned freq_hz)
 {
@@ -1241,6 +1248,7 @@ Clock *machine_create_constant_clock(MachineState *machine,
 
     clk = clock_new(OBJECT(machine), name);
     clock_set_hz(clk, freq_hz);
+    qemu_register_reset(constant_clock_reset, clk);
 
     return clk;
 }
-- 
2.26.3



^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (8 preceding siblings ...)
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset() Philippe Mathieu-Daudé
@ 2021-04-09 13:12 ` Philippe Mathieu-Daudé
  2021-04-09 14:11   ` Philippe Mathieu-Daudé
  2021-04-10 13:19 ` Luc Michel
  10 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09 13:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	qemu-arm, Paolo Bonzini

On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
> Hi Damian, Luc, Peter.
> 
> I've been debugging some odd issue with the clocks:
> a clock created in the machine (IOW, not a qdev clock) isn't
> always resetted, thus propagating its value.
> "not always" is the odd part. In the MPS2 board, the machine
> clock is propagated. Apparently because the peripherals are
> created directly in the machine_init() handler. When moving
> them out in a SoC QOM container, the clock isn't... I'm still
> having hard time to understand what is going on.
> 
> Alternatively I tried to strengthen the clock API by reducing
> the clock creation in 2 cases: machine/device. This way clocks
> aren't left dangling around alone. The qdev clocks are properly
> resetted, and for the machine clocks I register a generic reset
> handler. This way is safer, but I don't think we want to keep
> adding generic reset handlers, instead we'd like to remove them.
> 
> I'll keep debugging to understand. Meanwhile posting this series
> as RFC to get feedback on the approach and start discussing on
> this issue.

I wonder if this could be the culprit:

  commit 96250eab904261b31d9d1ac3abbdb36737635ffa
  Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
  Date:   Fri Aug 28 10:02:44 2020 +0100

      hw/clock: Only propagate clock changes if the clock is changed

      Avoid propagating the clock change when the clock does not change.

  diff --git a/include/hw/clock.h b/include/hw/clock.h
  index d85af45c967..9ecd78b2c30 100644
  --- a/include/hw/clock.h
  +++ b/include/hw/clock.h
  @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
    */
   static inline void clock_update(Clock *clk, uint64_t value)
   {
  -    clock_set(clk, value);
  -    clock_propagate(clk);
  +    if (clock_set(clk, value)) {
  +        clock_propagate(clk);
  +    }
   }

I.e.:

- first use clock_set() to set the new period
- then call clock_update() with the same "new period"

-> the clock parent already has the new period, so the
   children are not updated.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-09 13:12 ` [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
@ 2021-04-09 14:11   ` Philippe Mathieu-Daudé
  2021-04-09 14:20     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09 14:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	qemu-arm, Paolo Bonzini

On 4/9/21 3:12 PM, Philippe Mathieu-Daudé wrote:
> On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
>> Hi Damian, Luc, Peter.
>>
>> I've been debugging some odd issue with the clocks:
>> a clock created in the machine (IOW, not a qdev clock) isn't
>> always resetted, thus propagating its value.
>> "not always" is the odd part. In the MPS2 board, the machine
>> clock is propagated. Apparently because the peripherals are
>> created directly in the machine_init() handler. When moving
>> them out in a SoC QOM container, the clock isn't... I'm still
>> having hard time to understand what is going on.
>>
>> Alternatively I tried to strengthen the clock API by reducing
>> the clock creation in 2 cases: machine/device. This way clocks
>> aren't left dangling around alone. The qdev clocks are properly
>> resetted, and for the machine clocks I register a generic reset
>> handler. This way is safer, but I don't think we want to keep
>> adding generic reset handlers, instead we'd like to remove them.
>>
>> I'll keep debugging to understand. Meanwhile posting this series
>> as RFC to get feedback on the approach and start discussing on
>> this issue.
> 
> I wonder if this could be the culprit:

No (same reverting it) :(

>   commit 96250eab904261b31d9d1ac3abbdb36737635ffa
>   Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>   Date:   Fri Aug 28 10:02:44 2020 +0100
> 
>       hw/clock: Only propagate clock changes if the clock is changed
> 
>       Avoid propagating the clock change when the clock does not change.
> 
>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>   index d85af45c967..9ecd78b2c30 100644
>   --- a/include/hw/clock.h
>   +++ b/include/hw/clock.h
>   @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
>     */
>    static inline void clock_update(Clock *clk, uint64_t value)
>    {
>   -    clock_set(clk, value);
>   -    clock_propagate(clk);
>   +    if (clock_set(clk, value)) {
>   +        clock_propagate(clk);
>   +    }
>    }
> 
> I.e.:
> 
> - first use clock_set() to set the new period
> - then call clock_update() with the same "new period"
> 
> -> the clock parent already has the new period, so the
>    children are not updated.

This is actually what clock_set_source() does:

  void clock_set_source(Clock *clk, Clock *src)
  {
      ...

      clk->period = src->period; // <------------------------------
      QLIST_INSERT_HEAD(&src->children, clk, sibling);
      clk->source = src;
      clock_propagate_period(clk, false);
  }

So indeed if we use qdev_connect_clock_in() in DeviceRealize(),
it calls clock_set_source() and set the period, does not propagate,
then later when clock_propagate_period() is called:

static void clock_propagate_period(Clock *clk, bool call_callbacks)
{
    ...
    QLIST_FOREACH(child, &clk->children, sibling) {
        if (child->period != clk->period) {
            //           ^^^^ this condition is false
            ...
            clock_propagate_period(child, call_callbacks);
            // ^^^ children never get clock propagated
        }
    }
}

Does it make sense?


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-09 14:11   ` Philippe Mathieu-Daudé
@ 2021-04-09 14:20     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09 14:20 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Peter Maydell, Luc Michel, Eduardo Habkost,
	qemu-arm, Paolo Bonzini

On 4/9/21 4:11 PM, Philippe Mathieu-Daudé wrote:
> On 4/9/21 3:12 PM, Philippe Mathieu-Daudé wrote:
>> On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Damian, Luc, Peter.
>>>
>>> I've been debugging some odd issue with the clocks:
>>> a clock created in the machine (IOW, not a qdev clock) isn't
>>> always resetted, thus propagating its value.
>>> "not always" is the odd part. In the MPS2 board, the machine
>>> clock is propagated. Apparently because the peripherals are
>>> created directly in the machine_init() handler. When moving
>>> them out in a SoC QOM container, the clock isn't... I'm still
>>> having hard time to understand what is going on.
>>>
>>> Alternatively I tried to strengthen the clock API by reducing
>>> the clock creation in 2 cases: machine/device. This way clocks
>>> aren't left dangling around alone. The qdev clocks are properly
>>> resetted, and for the machine clocks I register a generic reset
>>> handler. This way is safer, but I don't think we want to keep
>>> adding generic reset handlers, instead we'd like to remove them.
>>>
>>> I'll keep debugging to understand. Meanwhile posting this series
>>> as RFC to get feedback on the approach and start discussing on
>>> this issue.
>>
>> I wonder if this could be the culprit:
> 
> No (same reverting it) :(
> 
>>   commit 96250eab904261b31d9d1ac3abbdb36737635ffa
>>   Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>   Date:   Fri Aug 28 10:02:44 2020 +0100
>>
>>       hw/clock: Only propagate clock changes if the clock is changed
>>
>>       Avoid propagating the clock change when the clock does not change.
>>
>>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>>   index d85af45c967..9ecd78b2c30 100644
>>   --- a/include/hw/clock.h
>>   +++ b/include/hw/clock.h
>>   @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
>>     */
>>    static inline void clock_update(Clock *clk, uint64_t value)
>>    {
>>   -    clock_set(clk, value);
>>   -    clock_propagate(clk);
>>   +    if (clock_set(clk, value)) {
>>   +        clock_propagate(clk);
>>   +    }
>>    }
>>
>> I.e.:
>>
>> - first use clock_set() to set the new period
>> - then call clock_update() with the same "new period"
>>
>> -> the clock parent already has the new period, so the
>>    children are not updated.
> 
> This is actually what clock_set_source() does:
> 
>   void clock_set_source(Clock *clk, Clock *src)
>   {
>       ...
> 
>       clk->period = src->period; // <------------------------------
>       QLIST_INSERT_HEAD(&src->children, clk, sibling);
>       clk->source = src;
>       clock_propagate_period(clk, false);
>   }
> 
> So indeed if we use qdev_connect_clock_in() in DeviceRealize(),
> it calls clock_set_source() and set the period, does not propagate,
> then later when clock_propagate_period() is called:
> 
> static void clock_propagate_period(Clock *clk, bool call_callbacks)
> {
>     ...
>     QLIST_FOREACH(child, &clk->children, sibling) {
>         if (child->period != clk->period) {
>             //           ^^^^ this condition is false
>             ...
>             clock_propagate_period(child, call_callbacks);
>             // ^^^ children never get clock propagated
>         }
>     }
> }
> 
> Does it make sense?

FWIW this fixes the problem I'm having:

-- >8 --
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 23c0c372b5d..4ecb51b1465 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -87,7 +87,7 @@ static void clock_propagate_period(Clock *clk, bool
call_callbacks)
                                        CLOCK_PERIOD_TO_HZ(clk->period),
                                        CLOCK_PATH(child),
                                        CLOCK_PERIOD_TO_HZ(child->period));
-        if (child->period != clk->period) {
+        if (1) {//child->period != clk->period) {
             if (call_callbacks) {
                 clock_call_callback(child, ClockPreUpdate);
             }
---

At least this confirm my hypothesis and reduces the scope of
the problem.

I'm not sure what is the best way to fix this (yet?) so I'll
wait here for feedback. At least I can keep going :)

Regards,

Phil.


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
                   ` (9 preceding siblings ...)
  2021-04-09 13:12 ` [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
@ 2021-04-10 13:19 ` Luc Michel
  2021-04-10 13:53   ` Philippe Mathieu-Daudé
  10 siblings, 1 reply; 30+ messages in thread
From: Luc Michel @ 2021-04-10 13:19 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Eduardo Habkost, qemu-devel,
	qemu-arm, Paolo Bonzini

Hi Philippe,

On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
> Hi Damian, Luc, Peter.
> 
> I've been debugging some odd issue with the clocks:
> a clock created in the machine (IOW, not a qdev clock) isn't
> always resetted, thus propagating its value.
> "not always" is the odd part. In the MPS2 board, the machine
> clock is propagated. Apparently because the peripherals are
> created directly in the machine_init() handler. When moving
> them out in a SoC QOM container, the clock isn't... I'm still
> having hard time to understand what is going on.

I think there is a misunderstanding on how the clock API works. If I
understand correctly your issue, you expect the callback of an input
clock connected to your constant "main oscillator" clock to be called on
machine reset.

If I'm not mistaken this is not the way the API has been designed. The
callback is called only when the clock period changes. A constant clock
does not change on reset, so the callback of child clocks should not be
called.

However devices that care about this clock value (e.g. a device that
has a clock input connected to this constant clock) should see their
standard reset callback called during reset. And they can effectively read
the clock value here and do what they need to do.

Note that clock propagation during reset has always been a complicated
problem. Calling clock_propagate is forbidden during the reset's enter
phase because of the side effects it can introduce.

> 
> Alternatively I tried to strengthen the clock API by reducing
> the clock creation in 2 cases: machine/device. This way clocks
> aren't left dangling around alone. The qdev clocks are properly
> resetted, and for the machine clocks I register a generic reset
> handler. This way is safer, but I don't think we want to keep
> adding generic reset handlers, instead we'd like to remove them.

I find your API modification a bit restrictive. I think creating a
standalone clock can be useful, e.g. in complicated devices that may
want to use internal "intermediate" clocks. I would not remove this
possibility to the API users.

Thanks,

-- 
Luc


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-10 13:19 ` Luc Michel
@ 2021-04-10 13:53   ` Philippe Mathieu-Daudé
  2021-04-10 15:15     ` Peter Maydell
  2021-04-19 19:39     ` Luc Michel
  0 siblings, 2 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-10 13:53 UTC (permalink / raw)
  To: Luc Michel
  Cc: Damien Hedde, Peter Maydell, Eduardo Habkost, qemu-devel,
	qemu-arm, Paolo Bonzini

Hi Luc,

On 4/10/21 3:19 PM, Luc Michel wrote:
> On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
>> I've been debugging some odd issue with the clocks:
>> a clock created in the machine (IOW, not a qdev clock) isn't
>> always resetted, thus propagating its value.
>> "not always" is the odd part. In the MPS2 board, the machine
>> clock is propagated. Apparently because the peripherals are
>> created directly in the machine_init() handler. When moving
>> them out in a SoC QOM container, the clock isn't... I'm still
>> having hard time to understand what is going on.
> 
> I think there is a misunderstanding on how the clock API works. If I
> understand correctly your issue, you expect the callback of an input
> clock connected to your constant "main oscillator" clock to be called on
> machine reset.
> 
> If I'm not mistaken this is not the way the API has been designed. The
> callback is called only when the clock period changes. A constant clock
> does not change on reset, so the callback of child clocks should not be
> called.

They why the children of a clock tree fed with constant clock stay with
a clock of 0? Who is responsible of setting their clock to the constant
value?

> However devices that care about this clock value (e.g. a device that
> has a clock input connected to this constant clock) should see their
> standard reset callback called during reset. And they can effectively read
> the clock value here and do what they need to do.
> 
> Note that clock propagation during reset has always been a complicated
> problem. Calling clock_propagate is forbidden during the reset's enter
> phase because of the side effects it can introduce.

Ah... Maybe this is related to the generic reset problem in QEMU :(

>> Alternatively I tried to strengthen the clock API by reducing
>> the clock creation in 2 cases: machine/device. This way clocks
>> aren't left dangling around alone. The qdev clocks are properly
>> resetted, and for the machine clocks I register a generic reset
>> handler. This way is safer, but I don't think we want to keep
>> adding generic reset handlers, instead we'd like to remove them.
> 
> I find your API modification a bit restrictive. I think creating a
> standalone clock can be useful, e.g. in complicated devices that may
> want to use internal "intermediate" clocks. I would not remove this
> possibility to the API users.

Well, this is the point. I can't see a justification to have a clock
on a non-qdev object. We should be able to model complicated devices
with qdev.

We are having various problems with the CPUs which are non-qdev devices,
or recently even with the LED model...:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg798031.html

Phil.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-10 13:53   ` Philippe Mathieu-Daudé
@ 2021-04-10 15:15     ` Peter Maydell
  2021-04-10 16:14       ` Philippe Mathieu-Daudé
  2021-04-12 10:11       ` Peter Maydell
  2021-04-19 19:39     ` Luc Michel
  1 sibling, 2 replies; 30+ messages in thread
From: Peter Maydell @ 2021-04-10 15:15 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Luc,
>
> On 4/10/21 3:19 PM, Luc Michel wrote:
> > Note that clock propagation during reset has always been a complicated
> > problem. Calling clock_propagate is forbidden during the reset's enter
> > phase because of the side effects it can introduce.
>
> Ah... Maybe this is related to the generic reset problem in QEMU :(

I do wonder if we got the clock-propagation-during-reset part of this
wrong -- it seemed right to me at the time but trying to use the
clock API recently I did run into some unhelpful-seeming results
(I forget the details now, though).

> > I find your API modification a bit restrictive. I think creating a
> > standalone clock can be useful, e.g. in complicated devices that may
> > want to use internal "intermediate" clocks. I would not remove this
> > possibility to the API users.
>
> Well, this is the point. I can't see a justification to have a clock
> on a non-qdev object. We should be able to model complicated devices
> with qdev.

The obvious reason is that machine objects are not qdev devices (ie
TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
but it's a reasonable thing to say "this machine has a fixed frequency
clock which it connects to the SoC".

I do wonder if the right fix to that would be to make TYPE_MACHINE
be a subtype of TYPE_DEVICE, though -- machines not being subtypes
of device has other annoying effects, like their not having reset
methods or being able to register vmstate structs. There might be
some unanticipated side effects of making that change, though.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-10 15:15     ` Peter Maydell
@ 2021-04-10 16:14       ` Philippe Mathieu-Daudé
  2021-04-12 10:11       ` Peter Maydell
  1 sibling, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-10 16:14 UTC (permalink / raw)
  To: Peter Maydell, Mark Cave-Ayland, Igor Mammedov, Eduardo Habkost
  Cc: Damien Hedde, Paolo Bonzini, qemu-arm, Luc Michel, QEMU Developers

+Mark & Igor

On 4/10/21 5:15 PM, Peter Maydell wrote:
> On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/10/21 3:19 PM, Luc Michel wrote:
>>> Note that clock propagation during reset has always been a complicated
>>> problem. Calling clock_propagate is forbidden during the reset's enter
>>> phase because of the side effects it can introduce.
>>
>> Ah... Maybe this is related to the generic reset problem in QEMU :(
> 
> I do wonder if we got the clock-propagation-during-reset part of this
> wrong -- it seemed right to me at the time but trying to use the
> clock API recently I did run into some unhelpful-seeming results
> (I forget the details now, though).
> 
>>> I find your API modification a bit restrictive. I think creating a
>>> standalone clock can be useful, e.g. in complicated devices that may
>>> want to use internal "intermediate" clocks. I would not remove this
>>> possibility to the API users.
>>
>> Well, this is the point. I can't see a justification to have a clock
>> on a non-qdev object. We should be able to model complicated devices
>> with qdev.
> 
> The obvious reason is that machine objects are not qdev devices (ie
> TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE),
> but it's a reasonable thing to say "this machine has a fixed frequency
> clock which it connects to the SoC".

In this series I restrict Clocks to 1/ qdev and 2/ MachineState (which
is the case you described). I tried to think about all the hardware I
worked with and all could be somehow modeled as qdev.

> I do wonder if the right fix to that would be to make TYPE_MACHINE
> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
> of device has other annoying effects, like their not having reset
> methods or being able to register vmstate structs. There might be
> some unanticipated side effects of making that change, though.

Yes, that would simplify few things. I might try it.

Expanding the topic, this reminds me a discussion between Igor and Mark
about MemoryRegion...

One was about we can not change the NULL owner to MachineState
because the region could be migrated and there is a mismatch.

Another was about preparing the design for multi-arch machines, Mark was
confuse by having owner for memory regions such DRAM because on a board
they aren't owned, can be used by various masters (CPUs, DMA). So the
machine should be the owner somehow. Maybe the problem was with
migration indeed, I can't remember :S

Regards,

Phil.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-10 15:15     ` Peter Maydell
  2021-04-10 16:14       ` Philippe Mathieu-Daudé
@ 2021-04-12 10:11       ` Peter Maydell
  2021-04-12 10:31         ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2021-04-12 10:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

On Sat, 10 Apr 2021 at 16:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> I do wonder if the right fix to that would be to make TYPE_MACHINE
> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
> of device has other annoying effects, like their not having reset
> methods or being able to register vmstate structs.

I wasn't quite right about this -- turns out that machines can
have a reset method, but it is a weird special case method
MachineClass::reset that isn't like device reset. (We use it
in a few machine types; if implemented, it is responsible for
calling qemu_devices_reset() to kick off device reset. Probably
MachineClass ought to implement TYPE_RESETTABLE_INTERFACE
instead.)

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-12 10:11       ` Peter Maydell
@ 2021-04-12 10:31         ` Philippe Mathieu-Daudé
  2021-04-12 10:44           ` Peter Maydell
  0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-12 10:31 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

Hi Peter,

On 4/12/21 12:11 PM, Peter Maydell wrote:
> On Sat, 10 Apr 2021 at 16:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I do wonder if the right fix to that would be to make TYPE_MACHINE
>> be a subtype of TYPE_DEVICE, though -- machines not being subtypes
>> of device has other annoying effects, like their not having reset
>> methods or being able to register vmstate structs.
> 
> I wasn't quite right about this -- turns out that machines can
> have a reset method, but it is a weird special case method
> MachineClass::reset that isn't like device reset. (We use it
> in a few machine types; if implemented, it is responsible for
> calling qemu_devices_reset() to kick off device reset. Probably
> MachineClass ought to implement TYPE_RESETTABLE_INTERFACE
> instead.)

TIL MachineClass::reset().

- hw/hppa/machine.c
- hw/i386/pc.c

  Used to reset CPUs manually because CPUs aren't sysbus-reset.

- hw/i386/microvm.c

  Removing the microvm_fix_kernel_cmdline() call which is suspicious
  at reset (should be enough once in realize time), the is the same
  previous "Used to reset CPUs manually ..."

- hw/ppc/pnv.c

  "Reset BMC simulator" seems a legitimate case of machine reset.

  Next is 'fdt = pnv_dt_create()'. So guest has changed hardware state
  and we need to refresh the DT for the next guest boots. Could be.

- hw/ppc/spapr.c

  Similar previous "Used to reset CPUs manually ..."

  Similar previous "update DT after hardware physically changed".


Am I correct than half of these handlers code is to kludge the fact
that CPU aren't reset such device reset?

Regards,

Phil.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-12 10:31         ` Philippe Mathieu-Daudé
@ 2021-04-12 10:44           ` Peter Maydell
  2021-04-12 11:00             ` Philippe Mathieu-Daudé
  2021-04-13 19:43             ` Eduardo Habkost
  0 siblings, 2 replies; 30+ messages in thread
From: Peter Maydell @ 2021-04-12 10:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> TIL MachineClass::reset().
>
> - hw/hppa/machine.c
> - hw/i386/pc.c
>
>   Used to reset CPUs manually because CPUs aren't sysbus-reset.

pc_machine_reset() is not resetting the CPUs -- it is
re-resetting the APIC devices, which looks like it is a
workaround for a reset-ordering or other problem. I'm not
sure where the CPUs are being reset...

-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-12 10:44           ` Peter Maydell
@ 2021-04-12 11:00             ` Philippe Mathieu-Daudé
  2021-04-13 19:43             ` Eduardo Habkost
  1 sibling, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-12 11:00 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

On 4/12/21 12:44 PM, Peter Maydell wrote:
> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> TIL MachineClass::reset().
>>
>> - hw/hppa/machine.c
>> - hw/i386/pc.c
>>
>>   Used to reset CPUs manually because CPUs aren't sysbus-reset.
> 
> pc_machine_reset() is not resetting the CPUs -- it is
> re-resetting the APIC devices, which looks like it is a
> workaround for a reset-ordering or other problem. I'm not
> sure where the CPUs are being reset...

Ah, I got confused by the CPU_FOREACH(cs) macro.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-12 10:44           ` Peter Maydell
  2021-04-12 11:00             ` Philippe Mathieu-Daudé
@ 2021-04-13 19:43             ` Eduardo Habkost
  2021-05-03 15:20               ` Igor Mammedov
  1 sibling, 1 reply; 30+ messages in thread
From: Eduardo Habkost @ 2021-04-13 19:43 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Damien Hedde, Luc Michel, Philippe Mathieu-Daudé,
	QEMU Developers, qemu-arm, Igor Mammedov, Paolo Bonzini

On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > TIL MachineClass::reset().
> >
> > - hw/hppa/machine.c
> > - hw/i386/pc.c
> >
> >   Used to reset CPUs manually because CPUs aren't sysbus-reset.
> 
> pc_machine_reset() is not resetting the CPUs -- it is
> re-resetting the APIC devices, which looks like it is a
> workaround for a reset-ordering or other problem. I'm not
> sure where the CPUs are being reset...

CPU reset code was moved from pc.c:pc_cpu_reset() to
cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
("target-i386: move cpu_reset and reset callback to cpu.c").
It's not clear to me why.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper Philippe Mathieu-Daudé
@ 2021-04-19 14:22   ` Peter Maydell
  0 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2021-04-19 14:22 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Daniel P. Berrangé,
	Paolo Bonzini

On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Clocks are rarely left unconnected, but rather connected to ground
> plane to avoid noise. When representing the clock tree, we want to
> see such ground clock. As we might reuse this clock on various
> boards, introduce the qdev_ground_clock() which return a singleton
> ground clock.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/qdev-clock.h |  9 +++++++++
>  hw/core/qdev-clock.c    | 11 +++++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/include/hw/qdev-clock.h b/include/hw/qdev-clock.h
> index ffa0f7ba09e..2f2d2da6cd6 100644
> --- a/include/hw/qdev-clock.h
> +++ b/include/hw/qdev-clock.h
> @@ -161,4 +161,13 @@ typedef struct ClockPortInitElem ClockPortInitArray[];
>   */
>  void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks);
>
> +/**
> + * qdev_ground_clock:
> + * @returns: a pointer to the ground clock
> + *
> + * Get the special 'ground' clock. This clock can be used as input
> + * (unclocked) or output.
> + */
> +Clock *qdev_ground_clock(void);
> +
>  #endif /* QDEV_CLOCK_H */
> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
> index 117f4c6ea4a..a46384a84b7 100644
> --- a/hw/core/qdev-clock.c
> +++ b/hw/core/qdev-clock.c
> @@ -210,3 +210,14 @@ void qdev_connect_clock_in(DeviceState *dev, const char *name, Clock *source)
>      assert(!dev->realized);
>      clock_set_source(qdev_get_clock_in(dev, name), source);
>  }
> +
> +Clock *qdev_ground_clock(void)
> +{
> +    static Clock *gnd_clk;
> +
> +    if (!gnd_clk) {
> +        gnd_clk = clock_new(qdev_get_machine(), "gnd");
> +    }
> +
> +    return gnd_clk;
> +}

I'm not really convinced that we need this, given that we have
exactly one user of it in the tree. I think if you happen to want
a fixed-frequency-0Hz clock it's not a big deal to just create
it like any other clock. If we had half a dozen users in the tree
I might feel differently.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board
  2021-04-09  6:23 ` [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board Philippe Mathieu-Daudé
@ 2021-04-19 14:24   ` Peter Maydell
  0 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2021-04-19 14:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	Andrew Baumann, qemu-arm, Paolo Bonzini

On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The XOsc signal isn't a part of the CPRMAN, but comes from a
> crystal oscillator external to the SoC.
>
> Create the oscillator on the board, and propagate it to the
> CPRMAN.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally Philippe Mathieu-Daudé
@ 2021-04-19 14:26   ` Peter Maydell
  2021-04-20  9:27     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 30+ messages in thread
From: Peter Maydell @ 2021-04-19 14:26 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Daniel P. Berrangé,
	Paolo Bonzini

On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> To enforce correct API usage, restrict the clock creation to
> hw/core/. The only possible ways to create a clock are:
>
> - Constant clock at the board level
>   Using machine_create_constant_clock() in machine_init()
>
> - Propagated clock in QDev
>   Using qdev_init_clock_in() or qdev_init_clock_out() in
>   TYPE_DEVICE instance_init().

Why isn't it OK to have a constant clock inside a device ?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset()
  2021-04-09  6:24 ` [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset() Philippe Mathieu-Daudé
@ 2021-04-19 14:27   ` Peter Maydell
  0 siblings, 0 replies; 30+ messages in thread
From: Peter Maydell @ 2021-04-19 14:27 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Luc Michel, Eduardo Habkost, QEMU Developers,
	qemu-arm, Paolo Bonzini

On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> While the documentation mentions:
>
>   Note that if you are creating a clock with a fixed period which
>   will never change (for example the main clock source of a board),
>   then you'll have nothing else to do. This value will be propagated
>   to other clocks when connecting the clocks together and devices
>   will fetch the right value during the first reset.
>
> the clocks created in machine_init() aren't propagating their value
> because they are never reset (not part of the reset tree, such
> TYPE_DEVICE).
>
> Register a generic reset handler to have them properly reset.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/machine.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e8bdcd10854..2817fe6a567 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1234,6 +1234,13 @@ void machine_run_board_init(MachineState *machine)
>      phase_advance(PHASE_MACHINE_INITIALIZED);
>  }
>
> +static void constant_clock_reset(void *opaque)
> +{
> +    Clock *clk = opaque;
> +
> +    clock_propagate(clk);
> +}
> +
>  Clock *machine_create_constant_clock(MachineState *machine,
>                                       const char *name, unsigned freq_hz)
>  {
> @@ -1241,6 +1248,7 @@ Clock *machine_create_constant_clock(MachineState *machine,
>
>      clk = clock_new(OBJECT(machine), name);
>      clock_set_hz(clk, freq_hz);
> +    qemu_register_reset(constant_clock_reset, clk);

You mention this in the cover letter, but I agree that this
isn't really very nice. The machine's reset method ought to
reset the clocks (either explicitly or maybe some day implicitly).

thanks
-- PMM


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-10 13:53   ` Philippe Mathieu-Daudé
  2021-04-10 15:15     ` Peter Maydell
@ 2021-04-19 19:39     ` Luc Michel
  1 sibling, 0 replies; 30+ messages in thread
From: Luc Michel @ 2021-04-19 19:39 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Damien Hedde, Peter Maydell, Eduardo Habkost, qemu-devel,
	qemu-arm, Paolo Bonzini

On 15:53 Sat 10 Apr     , Philippe Mathieu-Daudé wrote:
> Hi Luc,
> 
> On 4/10/21 3:19 PM, Luc Michel wrote:
> > On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
> >> I've been debugging some odd issue with the clocks:
> >> a clock created in the machine (IOW, not a qdev clock) isn't
> >> always resetted, thus propagating its value.
> >> "not always" is the odd part. In the MPS2 board, the machine
> >> clock is propagated. Apparently because the peripherals are
> >> created directly in the machine_init() handler. When moving
> >> them out in a SoC QOM container, the clock isn't... I'm still
> >> having hard time to understand what is going on.
> > 
> > I think there is a misunderstanding on how the clock API works. If I
> > understand correctly your issue, you expect the callback of an input
> > clock connected to your constant "main oscillator" clock to be called on
> > machine reset.
> > 
> > If I'm not mistaken this is not the way the API has been designed. The
> > callback is called only when the clock period changes. A constant clock
> > does not change on reset, so the callback of child clocks should not be
> > called.
> 
> They why the children of a clock tree fed with constant clock stay with
> a clock of 0? Who is responsible of setting their clock to the constant
> value?

I think we expect the child to be set when we call clock_set_source. In
this function the child period is set to the parent one. Maybe you have
a case where clock_set_source is called before clock_set on the parent?

-- 
Luc


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally
  2021-04-19 14:26   ` Peter Maydell
@ 2021-04-20  9:27     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20  9:27 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Damien Hedde, Daniel P. Berrangé,
	Eduardo Habkost, QEMU Developers, qemu-arm, Paolo Bonzini,
	Luc Michel

On 4/19/21 4:26 PM, Peter Maydell wrote:
> On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> To enforce correct API usage, restrict the clock creation to
>> hw/core/. The only possible ways to create a clock are:
>>
>> - Constant clock at the board level
>>   Using machine_create_constant_clock() in machine_init()
>>
>> - Propagated clock in QDev
>>   Using qdev_init_clock_in() or qdev_init_clock_out() in
>>   TYPE_DEVICE instance_init().
> 
> Why isn't it OK to have a constant clock inside a device ?

I'm not an electronic engineer, so I guessed because I never used
a component which generate a clock source without being externally
excited by a crystal or oscillator. Such exciters are components
on the board. I might be wrong.

Using clock source out of qdev is giving us headache... So I'm
trying to enforce all clocks being used via qdev. Looking at the
resting cases and thinking about hardware, my understanding is
what's left belong to the "(constant) clock source on the board",
added this machine_create_constant_clock() method to complete the
enforced API.

Maybe what I'm trying to fix is a side-effect of the non-qdev reset
problem, and if we get a QOM tree reset, then a clock on a non-qdev
object would properly propagate its constant value to its children.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-04-13 19:43             ` Eduardo Habkost
@ 2021-05-03 15:20               ` Igor Mammedov
  2021-05-03 16:37                 ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 30+ messages in thread
From: Igor Mammedov @ 2021-05-03 15:20 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Damien Hedde, Peter Maydell, Luc Michel,
	Philippe Mathieu-Daudé,
	QEMU Developers, qemu-arm, Paolo Bonzini

On Tue, 13 Apr 2021 15:43:19 -0400
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
> > On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> > > TIL MachineClass::reset().
> > >
> > > - hw/hppa/machine.c
> > > - hw/i386/pc.c
> > >
> > >   Used to reset CPUs manually because CPUs aren't sysbus-reset.  
> > 
> > pc_machine_reset() is not resetting the CPUs -- it is
> > re-resetting the APIC devices, which looks like it is a
> > workaround for a reset-ordering or other problem. I'm not
> > sure where the CPUs are being reset...  
> 
> CPU reset code was moved from pc.c:pc_cpu_reset() to
> cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
> ("target-i386: move cpu_reset and reset callback to cpu.c").
> It's not clear to me why.

it was for cpu hotplug support, so that is we would have
CPU in well know initial state after realize is complete.



^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
  2021-05-03 15:20               ` Igor Mammedov
@ 2021-05-03 16:37                 ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-03 16:37 UTC (permalink / raw)
  To: Igor Mammedov, Eduardo Habkost
  Cc: Damien Hedde, Peter Maydell, Luc Michel, QEMU Developers,
	qemu-arm, Paolo Bonzini

On 5/3/21 5:20 PM, Igor Mammedov wrote:
> On Tue, 13 Apr 2021 15:43:19 -0400
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Mon, Apr 12, 2021 at 11:44:29AM +0100, Peter Maydell wrote:
>>> On Mon, 12 Apr 2021 at 11:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
>>>> TIL MachineClass::reset().
>>>>
>>>> - hw/hppa/machine.c
>>>> - hw/i386/pc.c
>>>>
>>>>   Used to reset CPUs manually because CPUs aren't sysbus-reset.  
>>>
>>> pc_machine_reset() is not resetting the CPUs -- it is
>>> re-resetting the APIC devices, which looks like it is a
>>> workaround for a reset-ordering or other problem. I'm not
>>> sure where the CPUs are being reset...  
>>
>> CPU reset code was moved from pc.c:pc_cpu_reset() to
>> cpu.c:x86_cpu_machine_reset_cb() in commit 65dee3805259
>> ("target-i386: move cpu_reset and reset callback to cpu.c").
>> It's not clear to me why.
> 
> it was for cpu hotplug support, so that is we would have
> CPU in well know initial state after realize is complete.

It makes sense, but I don't see why this is considered x86 specific.


^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-05-03 16:44 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09  6:23 [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 1/9] hw/core/clock: Increase clock propagation trace events verbosity Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 2/9] hw/core/machine: Add machine_create_constant_clock() helper Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 3/9] hw/arm: Use new " Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 4/9] hw/mips: " Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 5/9] hw/core/qdev-clock: Add qdev_ground_clock() helper Philippe Mathieu-Daudé
2021-04-19 14:22   ` Peter Maydell
2021-04-09  6:23 ` [RFC PATCH-for-6.1 6/9] hw/misc/bcm2835_cprman: Use " Philippe Mathieu-Daudé
2021-04-09  6:23 ` [RFC PATCH-for-6.1 7/9] hw/misc/bcm2835_cprman: Feed 'xosc' from the board Philippe Mathieu-Daudé
2021-04-19 14:24   ` Peter Maydell
2021-04-09  6:24 ` [RFC PATCH-for-6.1 8/9] hw/clock: Declare clock_new() internally Philippe Mathieu-Daudé
2021-04-19 14:26   ` Peter Maydell
2021-04-20  9:27     ` Philippe Mathieu-Daudé
2021-04-09  6:24 ` [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset() Philippe Mathieu-Daudé
2021-04-19 14:27   ` Peter Maydell
2021-04-09 13:12 ` [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation Philippe Mathieu-Daudé
2021-04-09 14:11   ` Philippe Mathieu-Daudé
2021-04-09 14:20     ` Philippe Mathieu-Daudé
2021-04-10 13:19 ` Luc Michel
2021-04-10 13:53   ` Philippe Mathieu-Daudé
2021-04-10 15:15     ` Peter Maydell
2021-04-10 16:14       ` Philippe Mathieu-Daudé
2021-04-12 10:11       ` Peter Maydell
2021-04-12 10:31         ` Philippe Mathieu-Daudé
2021-04-12 10:44           ` Peter Maydell
2021-04-12 11:00             ` Philippe Mathieu-Daudé
2021-04-13 19:43             ` Eduardo Habkost
2021-05-03 15:20               ` Igor Mammedov
2021-05-03 16:37                 ` Philippe Mathieu-Daudé
2021-04-19 19:39     ` Luc Michel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).