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

Commit Message

Damien Hedde Sept. 4, 2019, 12:55 p.m. UTC

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,
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 <[e-mail]>
Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
Tested-by: Philippe Mathieu-Daudé <[e-mail]>
---
 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

Comments

Philippe Mathieu-Daudé Nov. 25, 2019, 1:07 p.m. UTC | #1

On 9/4/19 2:55 PM, Damien Hedde 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,
> 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 <[e-mail]>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
>   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

2019 now?

> + *
> + * Authors:
> + *  Frederic Konrad <[e-mail]>
> + *  Damien Hedde <[e-mail]>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */

Philippe Mathieu-Daudé Nov. 25, 2019, 1:37 p.m. UTC | #2

On 9/4/19 2:55 PM, Damien Hedde 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,
> 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 <[e-mail]>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
>   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 <[e-mail]>
> + *  Damien Hedde <[e-mail]>
> + *
> + * 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)
> +
> +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);
> +
> +    clk->frequency = freq;
> +}
> +
> +void clock_clear_callback(ClockIn *clk)
> +{
> +    clock_set_callback(clk, NULL, NULL);
> +}
> +
> +void clock_connect(ClockIn *clkin, ClockOut *clkout)
> +{
> +    assert(clkin && clkin->driver == NULL);
> +    assert(clkout);
> +
> +    trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
> +
> +    QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
> +    clkin->driver = clkout;
> +}
> +
> +static void clock_disconnect(ClockIn *clk)
> +{
> +    if (clk->driver == NULL) {
> +        return;
> +    }
> +
> +    trace_clock_disconnect(CLOCK_PATH(clk));
> +
> +    clk->driver = NULL;
> +    QLIST_REMOVE(clk, sibling);
> +}
> +
> +void clock_set_frequency(ClockOut *clk, uint64_t freq)
> +{
> +    ClockIn *follower;
> +    trace_clock_set_frequency(CLOCK_PATH(clk), freq);
> +
> +    QLIST_FOREACH(follower, &clk->followers, sibling) {
> +        trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
> +        if (follower->frequency != freq) {
> +            follower->frequency = freq;
> +            if (follower->callback) {
> +                follower->callback(follower->callback_opaque);
> +            }
> +        }
> +    }
> +}
> +
> +static void clock_out_initfn(Object *obj)
> +{
> +    ClockOut *clk = CLOCK_OUT(obj);
> +
> +    QLIST_INIT(&clk->followers);
> +}
> +
> +static void clock_out_finalizefn(Object *obj)
> +{
> +    ClockOut *clk = CLOCK_OUT(obj);
> +    ClockIn *follower, *next;
> +
> +    /* clear our list of followers */
> +    QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
> +        clock_disconnect(follower);
> +    }
> +
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = NULL;
> +}
> +
> +static void clock_in_finalizefn(Object *obj)
> +{
> +    ClockIn *clk = CLOCK_IN(obj);
> +
> +    /* remove us from driver's followers list */
> +    clock_disconnect(clk);
> +
> +    g_free(clk->canonical_path);
> +    clk->canonical_path = NULL;
> +}
> +
> +static const TypeInfo clock_out_info = {
> +    .name              = TYPE_CLOCK_OUT,
> +    .parent            = TYPE_OBJECT,
> +    .instance_size     = sizeof(ClockOut),
> +    .instance_init     = clock_out_initfn,
> +    .instance_finalize = clock_out_finalizefn,
> +};
> +
> +static const TypeInfo clock_in_info = {
> +    .name              = TYPE_CLOCK_IN,
> +    .parent            = TYPE_OBJECT,
> +    .instance_size     = sizeof(ClockIn),
> +    .instance_finalize = clock_in_finalizefn,
> +};
> +
> +static void clock_register_types(void)
> +{
> +    type_register_static(&clock_in_info);
> +    type_register_static(&clock_out_info);
> +}
> +
> +type_init(clock_register_types)
> 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.c"

> +clock_connect(const char *clk, const char *driver) "'%s' drived-by '%s'"
> +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
> +
> +#include "qom/object.h"
> +#include "qemu/queue.h"
> +
> +#define TYPE_CLOCK_IN "clock-in"
> +#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
> +#define TYPE_CLOCK_OUT "clock-out"
> +#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
> +
> +typedef void ClockCallback(void *opaque);
> +
> +typedef struct ClockOut ClockOut;
> +typedef struct ClockIn ClockIn;
> +
> +struct ClockIn {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< private >*/
> +    uint64_t frequency;
> +    char *canonical_path; /* clock path cache */
> +    ClockOut *driver; /* clock output controlling this clock */
> +    ClockCallback *callback; /* local callback */
> +    void *callback_opaque; /* opaque argument for the callback */
> +    QLIST_ENTRY(ClockIn) sibling;  /* entry in a followers list */
> +};
> +
> +struct ClockOut {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< private >*/
> +    char *canonical_path; /* clock path cache */
> +    QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
> +};

Can we keep the structure definitions opaque in hw/core/clock.c?
If so, clock_get_frequency() can't be inlined anymore.

> +
> +/**
> + * clock_out_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_out_setup_canonical_path(ClockOut *clk);
> +
> +/**
> + * clock_in_setup_canonical_path:
> + * @clk: clock
> + *
> + * compute the canonical path of the clock (used by log messages)
> + */
> +void clock_in_setup_canonical_path(ClockIn *clk);
> +
> +/**
> + * clock_add_callback:
> + * @clk: the clock to register the callback into
> + * @cb: the callback function
> + * @opaque: the argument to the callback
> + *
> + * Register a callback called on every clock update.
> + */
> +void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
> +
> +/**
> + * clock_clear_callback:
> + * @clk: the clock to delete the callback from
> + *
> + * Unregister the callback registered with clock_set_callback.
> + */
> +void clock_clear_callback(ClockIn *clk);
> +
> +/**
> + * clock_init_frequency:
> + * @clk: the clock to initialize.
> + * @freq: the clock's frequency in Hz or 0 if unclocked.
> + *
> + * Initialize the local cached frequency value of @clk to @freq.
> + * Note: this function must only be called during device inititialization
> + * or migration.
> + */
> +void clock_init_frequency(ClockIn *clk, uint64_t freq);
> +
> +/**
> + * clock_connect:
> + * @clkin: the drived clock.
> + * @clkout: the driving clock.
> + *
> + * Setup @clkout to drive @clkin: Any @clkout update will be propagated
> + * to @clkin.
> + */
> +void clock_connect(ClockIn *clkin, ClockOut *clkout);
> +
> +/**
> + * clock_set_frequency:
> + * @clk: the clock to update.
> + * @freq: the new clock's frequency in Hz or 0 if unclocked.
> + *
> + * Update the @clk to the new @freq.
> + * This change will be propagated through registered clock inputs.
> + */
> +void clock_set_frequency(ClockOut *clk, uint64_t freq);
> +
> +/**
> + * 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;
> +}
> +
> +/**
> + * 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;
> +}
> +
> +#endif /* QEMU_HW_CLOCK_H */
>

Peter Maydell Dec. 2, 2019, 1:42 p.m. UTC | #3

On Wed, 4 Sep 2019 at 13:56, Damien Hedde <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
>  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 <[e-mail]>
> + *  Damien Hedde <[e-mail]>
> + *
> + * 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

Damien Hedde Dec. 3, 2019, 3:14 p.m. UTC | #4

On 11/25/19 2:37 PM, Philippe Mathieu-Daudé wrote:
> On 9/4/19 2:55 PM, Damien Hedde 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,
>> 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 <[e-mail]>
>> Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>
>> Tested-by: Philippe Mathieu-Daudé <[e-mail]>
>> ---
>>   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/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.c"
> 

Oups, I missed this one in the renaming.

>> +
>> +struct ClockIn {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +    /*< private >*/
>> +    uint64_t frequency;
>> +    char *canonical_path; /* clock path cache */
>> +    ClockOut *driver; /* clock output controlling this clock */
>> +    ClockCallback *callback; /* local callback */
>> +    void *callback_opaque; /* opaque argument for the callback */
>> +    QLIST_ENTRY(ClockIn) sibling;  /* entry in a followers list */
>> +};
>> +
>> +struct ClockOut {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +    /*< private >*/
>> +    char *canonical_path; /* clock path cache */
>> +    QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
>> +};
> 
> Can we keep the structure definitions opaque in hw/core/clock.c?
> If so, clock_get_frequency() can't be inlined anymore.
> 

I think so. Apart from the monitor command (and the inline), nothing
requires the structure fields. I suppose we can add a function to access
or print the fields to be used by the monitor.

I don't have a opinion on this.

Damien

Damien Hedde Dec. 3, 2019, 3:28 p.m. UTC | #5

On 12/2/19 2:42 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <[e-mail]> 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.
>>
>> +
>> +#define CLOCK_PATH(_clk) (_clk->canonical_path)
> 
> Don't use leading underscores in identifiers, please.

ok

> 
>> +
>> +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).

I'll take a look.

> 
>> 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?).

Sure.

> 
>> +
> 
>> +/**
>> + * 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 ?

No, it's probably a remnant of previous version where input and output
shared some code. I'll remove it.

--
Damien

Patch

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 <[e-mail]>
+ *  Damien Hedde <[e-mail]>
+ *
+ * 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)
+
+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);
+
+    clk->frequency = freq;
+}
+
+void clock_clear_callback(ClockIn *clk)
+{
+    clock_set_callback(clk, NULL, NULL);
+}
+
+void clock_connect(ClockIn *clkin, ClockOut *clkout)
+{
+    assert(clkin && clkin->driver == NULL);
+    assert(clkout);
+
+    trace_clock_connect(CLOCK_PATH(clkin), CLOCK_PATH(clkout));
+
+    QLIST_INSERT_HEAD(&clkout->followers, clkin, sibling);
+    clkin->driver = clkout;
+}
+
+static void clock_disconnect(ClockIn *clk)
+{
+    if (clk->driver == NULL) {
+        return;
+    }
+
+    trace_clock_disconnect(CLOCK_PATH(clk));
+
+    clk->driver = NULL;
+    QLIST_REMOVE(clk, sibling);
+}
+
+void clock_set_frequency(ClockOut *clk, uint64_t freq)
+{
+    ClockIn *follower;
+    trace_clock_set_frequency(CLOCK_PATH(clk), freq);
+
+    QLIST_FOREACH(follower, &clk->followers, sibling) {
+        trace_clock_propagate(CLOCK_PATH(clk), CLOCK_PATH(follower));
+        if (follower->frequency != freq) {
+            follower->frequency = freq;
+            if (follower->callback) {
+                follower->callback(follower->callback_opaque);
+            }
+        }
+    }
+}
+
+static void clock_out_initfn(Object *obj)
+{
+    ClockOut *clk = CLOCK_OUT(obj);
+
+    QLIST_INIT(&clk->followers);
+}
+
+static void clock_out_finalizefn(Object *obj)
+{
+    ClockOut *clk = CLOCK_OUT(obj);
+    ClockIn *follower, *next;
+
+    /* clear our list of followers */
+    QLIST_FOREACH_SAFE(follower, &clk->followers, sibling, next) {
+        clock_disconnect(follower);
+    }
+
+    g_free(clk->canonical_path);
+    clk->canonical_path = NULL;
+}
+
+static void clock_in_finalizefn(Object *obj)
+{
+    ClockIn *clk = CLOCK_IN(obj);
+
+    /* remove us from driver's followers list */
+    clock_disconnect(clk);
+
+    g_free(clk->canonical_path);
+    clk->canonical_path = NULL;
+}
+
+static const TypeInfo clock_out_info = {
+    .name              = TYPE_CLOCK_OUT,
+    .parent            = TYPE_OBJECT,
+    .instance_size     = sizeof(ClockOut),
+    .instance_init     = clock_out_initfn,
+    .instance_finalize = clock_out_finalizefn,
+};
+
+static const TypeInfo clock_in_info = {
+    .name              = TYPE_CLOCK_IN,
+    .parent            = TYPE_OBJECT,
+    .instance_size     = sizeof(ClockIn),
+    .instance_finalize = clock_in_finalizefn,
+};
+
+static void clock_register_types(void)
+{
+    type_register_static(&clock_in_info);
+    type_register_static(&clock_out_info);
+}
+
+type_init(clock_register_types)
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'"
+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
+
+#include "qom/object.h"
+#include "qemu/queue.h"
+
+#define TYPE_CLOCK_IN "clock-in"
+#define CLOCK_IN(obj) OBJECT_CHECK(ClockIn, (obj), TYPE_CLOCK_IN)
+#define TYPE_CLOCK_OUT "clock-out"
+#define CLOCK_OUT(obj) OBJECT_CHECK(ClockOut, (obj), TYPE_CLOCK_OUT)
+
+typedef void ClockCallback(void *opaque);
+
+typedef struct ClockOut ClockOut;
+typedef struct ClockIn ClockIn;
+
+struct ClockIn {
+    /*< private >*/
+    Object parent_obj;
+    /*< private >*/
+    uint64_t frequency;
+    char *canonical_path; /* clock path cache */
+    ClockOut *driver; /* clock output controlling this clock */
+    ClockCallback *callback; /* local callback */
+    void *callback_opaque; /* opaque argument for the callback */
+    QLIST_ENTRY(ClockIn) sibling;  /* entry in a followers list */
+};
+
+struct ClockOut {
+    /*< private >*/
+    Object parent_obj;
+    /*< private >*/
+    char *canonical_path; /* clock path cache */
+    QLIST_HEAD(, ClockIn) followers; /* list of registered clocks */
+};
+
+/**
+ * clock_out_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_out_setup_canonical_path(ClockOut *clk);
+
+/**
+ * clock_in_setup_canonical_path:
+ * @clk: clock
+ *
+ * compute the canonical path of the clock (used by log messages)
+ */
+void clock_in_setup_canonical_path(ClockIn *clk);
+
+/**
+ * clock_add_callback:
+ * @clk: the clock to register the callback into
+ * @cb: the callback function
+ * @opaque: the argument to the callback
+ *
+ * Register a callback called on every clock update.
+ */
+void clock_set_callback(ClockIn *clk, ClockCallback *cb, void *opaque);
+
+/**
+ * clock_clear_callback:
+ * @clk: the clock to delete the callback from
+ *
+ * Unregister the callback registered with clock_set_callback.
+ */
+void clock_clear_callback(ClockIn *clk);
+
+/**
+ * clock_init_frequency:
+ * @clk: the clock to initialize.
+ * @freq: the clock's frequency in Hz or 0 if unclocked.
+ *
+ * Initialize the local cached frequency value of @clk to @freq.
+ * Note: this function must only be called during device inititialization
+ * or migration.
+ */
+void clock_init_frequency(ClockIn *clk, uint64_t freq);
+
+/**
+ * clock_connect:
+ * @clkin: the drived clock.
+ * @clkout: the driving clock.
+ *
+ * Setup @clkout to drive @clkin: Any @clkout update will be propagated
+ * to @clkin.
+ */
+void clock_connect(ClockIn *clkin, ClockOut *clkout);
+
+/**
+ * clock_set_frequency:
+ * @clk: the clock to update.
+ * @freq: the new clock's frequency in Hz or 0 if unclocked.
+ *
+ * Update the @clk to the new @freq.
+ * This change will be propagated through registered clock inputs.
+ */
+void clock_set_frequency(ClockOut *clk, uint64_t freq);
+
+/**
+ * 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;
+}
+
+/**
+ * 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;
+}
+
+#endif /* QEMU_HW_CLOCK_H */