Re: [PATCH v6 1/9] hw/core/clock: introduce clock objects

[Peter Maydell]

On Wed, 4 Sep 2019 at 13:56, Damien Hedde <address@hidden> wrote:
>
> Introduce clock objects: ClockIn and ClockOut.
>
> These objects may be used to distribute clocks from an object to several
> other objects. Each ClockIn object contains the current state of the
> clock: the frequency; it allows an object to migrate its input clock state
> independently of other objects.
>
> A ClockIn may be connected to a ClockOut so that it receives update,

"updates" (or "an update")

> through a callback, whenever the Clockout is updated using the
> ClockOut's set function.
>
> This is based on the original work of Frederic Konrad.
>
> Signed-off-by: Damien Hedde <address@hidden>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  Makefile.objs         |   1 +
>  hw/core/Makefile.objs |   1 +
>  hw/core/clock.c       | 144 ++++++++++++++++++++++++++++++++++++++++++
>  hw/core/trace-events  |   6 ++
>  include/hw/clock.h    | 124 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 276 insertions(+)
>  create mode 100644 hw/core/clock.c
>  create mode 100644 include/hw/clock.h
>
> diff --git a/Makefile.objs b/Makefile.objs
> index a723a47e14..4da623c759 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/audio
>  trace-events-subdirs += hw/block
>  trace-events-subdirs += hw/block/dataplane
>  trace-events-subdirs += hw/char
> +trace-events-subdirs += hw/core
>  trace-events-subdirs += hw/dma
>  trace-events-subdirs += hw/hppa
>  trace-events-subdirs += hw/i2c
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 69b408ad1c..c66a5b2c6b 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  common-obj-y += hotplug.o
> +common-obj-y += clock.o
>  common-obj-$(CONFIG_SOFTMMU) += nmi.o
>  common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
>
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> new file mode 100644
> index 0000000000..888f247f2a
> --- /dev/null
> +++ b/hw/core/clock.c
> @@ -0,0 +1,144 @@
> +/*
> + * Clock inputs and outputs
> + *
> + * Copyright GreenSocs 2016-2018
> + *
> + * Authors:
> + *  Frederic Konrad <address@hidden>
> + *  Damien Hedde <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/clock.h"
> +#include "trace.h"
> +
> +#define CLOCK_PATH(_clk) (_clk->canonical_path)

Don't use leading underscores in identifiers, please.

> +
> +void clock_out_setup_canonical_path(ClockOut *clk)
> +{
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_in_setup_canonical_path(ClockIn *clk)
> +{
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = object_get_canonical_path(OBJECT(clk));
> +}
> +
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque)
> +{
> +    assert(clk);
> +
> +    clk->callback = cb;
> +    clk->callback_opaque = opaque;
> +}
> +
> +void clock_init_frequency(ClockIn *clk, uint64_t freq)
> +{
> +    assert(clk);

This sort of assert isn't necessary. Asserts are good
when they help to make a bug visible sooner and more
obviously -- when they avoid "something goes wrong
much later on and further from the site of the actual
error". In this case, if the assert was not present
then the code would just segfault on the next line:

> +
> +    clk->frequency = freq;

which is already a very easy bug to diagnose and
where the offending caller will be in the backtrace.

If the parameter isn't supposed to be NULL, and the
method doesn't actually do anything that would
dereference it, that might be a good candidate to
assert on.

The same kind of unnecessary assert is also in some of
the other functions here (and probably in other patches).


> diff --git a/hw/core/trace-events b/hw/core/trace-events
> index ecf966c314..aa940e268b 100644
> --- a/hw/core/trace-events
> +++ b/hw/core/trace-events
> @@ -34,3 +34,9 @@ resettable_phase_hold_end(void *obj, int needed) "obj=%p 
> needed=%d"
>  resettable_phase_exit(void *obj, const char *type) "obj=%p(%s)"
>  resettable_phase_exit_end(void *obj, uint32_t count) "obj=%p count=%" PRIu32
>  resettable_count_underflow(void *obj) "obj=%p"
> +
> +# hw/core/clock-port.c
> +clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"

"driven-by"

> +clock_disconnect(const char *clk) "'%s'"
> +clock_set_frequency(const char *clk, uint64_t freq) "'%s' freq_hz=%" PRIu64
> +clock_propagate(const char *clko, const char *clki) "'%s' => '%s'"
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> new file mode 100644
> index 0000000000..fd11202ba4
> --- /dev/null
> +++ b/include/hw/clock.h
> @@ -0,0 +1,124 @@
> +#ifndef QEMU_HW_CLOCK_H
> +#define QEMU_HW_CLOCK_H

All new files need a copyright-and-license comment header (could
you check the rest of the patchset for this, please?).

> +

> +/**
> + * clock_get_frequency:
> + * @clk: the clk to fetch the clock
> + *
> + * @return: the current frequency of @clk in Hz. If @clk is NULL, return 0.
> + */
> +static inline uint64_t clock_get_frequency(const ClockIn *clk)
> +{
> +    return clk ? clk->frequency : 0;
> +}

Is there a use case where we want to support "pass in NULL"
rather than just making it a programming error for the caller
to try that ?

> +
> +/**
> + * clock_is_enabled:
> + * @clk: a clock state
> + *
> + * @return: true if the clock is running. If @clk is NULL return false.
> + */
> +static inline bool clock_is_enabled(const ClockIn *clk)
> +{
> +    return clock_get_frequency(clk) != 0;
> +}

Ditto here.

thanks
-- PMM