Re: [Qemu-arm] [PATCH v4 01/10] hw/core/clock-port: introduce clock port objects

[Peter Maydell]

On 17 September 2018 at 09:40,  <address@hidden> wrote:
> From: Damien Hedde <address@hidden>
>
> Introduce clock port objects: ClockIn and ClockOut.
>
> Theses ports may be used to distribute a clock from a object to several
> other objects. They do not contain any state and serve only as intermediate
> to carry a ClockState which contains 2 fields:
> * an integer which represent the frequency in Hertz
> * a boolean which represent the reset status of the clock domain
> Both are independent: eg the reset can be asserted while the clock running and
> vice versa.
>
> A ClockIn may be connected to a ClockOut so that it receives update,
> through the 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>
> ---
>  Makefile.objs           |   1 +
>  include/hw/clock-port.h | 153 ++++++++++++++++++++++++++++++++++++++++
>  hw/core/clock-port.c    | 145 +++++++++++++++++++++++++++++++++++++
>  hw/core/Makefile.objs   |   1 +
>  hw/core/trace-events    |   6 ++
>  5 files changed, 306 insertions(+)
>  create mode 100644 include/hw/clock-port.h
>  create mode 100644 hw/core/clock-port.c
>  create mode 100644 hw/core/trace-events
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ce9c79235e..b29747075f 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -210,6 +210,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/display
>  trace-events-subdirs += hw/dma
>  trace-events-subdirs += hw/hppa
> diff --git a/include/hw/clock-port.h b/include/hw/clock-port.h
> new file mode 100644
> index 0000000000..2ab554c30e
> --- /dev/null
> +++ b/include/hw/clock-port.h
> @@ -0,0 +1,153 @@
> +#ifndef CLOCK_PORT_H
> +#define CLOCK_PORT_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "qemu/queue.h"
> +
> +#define TYPE_CLOCK_PORT "clock-port"
> +#define CLOCK_PORT(obj) OBJECT_CHECK(ClockPort, (obj), TYPE_CLOCK_PORT)
> +#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 struct ClockState {
> +    uint64_t frequency; /* frequency of the clock in Hz */
> +    bool domain_reset; /* flag indicating if clock domain is under reset */
> +} ClockState;
> +
> +typedef void ClockCallback(void *opaque, ClockState *clk);
> +
> +typedef struct ClockPort {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< public >*/
> +    char *canonical_path; /* clock path shadow */
> +} ClockPort;

Having a ClockPort which ClockIn and ClockOut are derived
from seems a bit heavyweight. When would you want to operate on
a generic ClockPort rather than either a ClockIn or ClockOut ?
Why would you want the name (canonical_path) to be different
at the input and output ends -- is it possible to simplify
to just having the output end keep the name string ?

> --- /dev/null
> +++ b/hw/core/trace-events
> @@ -0,0 +1,6 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# 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_update(const char *clk, uint64_t freq, int reset) "'%s' frequency %" 
> PRIu64 "Hz reset %d"
> --
> 2.18.0
>

thanks
-- PMM