Re: [PATCH v7 1/9] hw/core/clock: introduce clock object

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

On 2/25/20 1:02 AM, Alistair Francis wrote:
> On Mon, Feb 24, 2020 at 9:05 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This object may be used to represent a clock inside a clock tree.
>>
>> A clock may be connected to another clock so that it receives update,
>> through a callback, whenever the source/parent clock is updated.
>>
>> Although only the root clock of a clock tree controls the values
>> (represented as periods) of all clocks in tree, each clock holds
>> a local state containing the current value so that it can be fetched
>> independently. It will allows us to fullfill migration requirements
>> by migrating each clock independently of others.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> --
>>
>> v7:
>> + merge ClockIn & ClockOut into a single type Clock
>> + switch clock state to a period with 2^-32ns unit
>> + add some Hz and ns helpers
>> + propagate clock period when setting the source so that
>>   clocks with fixed period are easy to handle.
>> ---
>>  include/hw/clock.h    | 216 ++++++++++++++++++++++++++++++++++++++++++
>>  hw/core/clock.c       | 131 +++++++++++++++++++++++++
>>  hw/core/Makefile.objs |   2 +
>>  hw/core/trace-events  |   7 ++
>>  4 files changed, 356 insertions(+)
>>  create mode 100644 include/hw/clock.h
>>  create mode 100644 hw/core/clock.c
>>
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> new file mode 100644
>> index 0000000000..30ac9a9946
>> --- /dev/null
>> +++ b/include/hw/clock.h
>> @@ -0,0 +1,216 @@

[...]

> 
>> +    clock_disconnect(clk);
>> +
>> +    g_free(clk->canonical_path);
>> +    clk->canonical_path = NULL;
> 
> You shouldn't need to set this to NULL.

ok.

> 
>> +}
>> +
>> +static const TypeInfo clock_info = {
>> +    .name              = TYPE_CLOCK,
>> +    .parent            = TYPE_OBJECT,
>> +    .instance_size     = sizeof(Clock),
>> +    .instance_init     = clock_initfn,
>> +    .instance_finalize = clock_finalizefn,
>> +};
>> +
>> +static void clock_register_types(void)
>> +{
>> +    type_register_static(&clock_info);
>> +}
>> +
>> +type_init(clock_register_types)
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index 6215e7c208..d7080edf89 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -7,10 +7,12 @@ common-obj-y += hotplug.o
>>  common-obj-y += vmstate-if.o
>>  # irq.o needed for qdev GPIO handling:
>>  common-obj-y += irq.o
>> +common-obj-y += clock.o
>>
>>  common-obj-$(CONFIG_SOFTMMU) += reset.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
>>  common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
>> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> 
> I don't think this should be here.
Oops, I missed this rebase artifact.

Thanks,
Damien