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

[Damien Hedde]

On 9/25/18 11:42 AM, Peter Maydell wrote:
> 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 ?

Here the canonical path is the full qom's canonical path. It is cached
in the state to avoid recomputing it when needed. It is only used for
log/trace purpose (without it, log is useless and calling
object_get_canonical_path at every log seemed too costly to me).
A connected ClockOut and ClockIn do not have the same (qom) parent,
their canonical path is different.
For example, in the zynq platform, there are 2 uarts, each one having a
 "refclk" input. There is also a clock controller (slcr) having 2
outputs "uart0_ref_clk" and "uart1_ref_clk". We end up with something
like this concerning canonical path:
+ output "[...]/slcr/uart0_ref_clk" drives input "[...]/uart0/refclk"
+ output "[...]/slcr/uart1_ref_clk" drives input "[...]/uart1/refclk"

Right now, I log a line for every end (output and input) when the clock
frequency change, which lead to a kind-of-duplicate log line (names
differ, but clock frequencies are obviously the same). I could only log
at the output end and drop the canonical path for the input or pust the
path in both objects.

Anyway it is easily possible to drop the ClockPort common ancestor. A
user never operates on it.

> 
>> --- /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
>