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

[Damien Hedde <damien.hedde@greensocs.com>]

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 <damien.hedde@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   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