| Message ID | 20210409062401.2350436-9-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,PATCH-for-6.1,1/9] hw/core/clock: Increase clock propagation trace events verbosity | expand |
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
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.
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
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