Re: [Qemu-devel] [PATCH v6 2/3] xlnx-zynqmp-rtc: Add basic time support

[Alistair Francis]

On Tue, Feb 27, 2018 at 8:36 AM, Peter Maydell <address@hidden> wrote:
> On 23 February 2018 at 17:21, Alistair Francis
> <address@hidden> wrote:
>> Allow the guest to determine the time set from the QEMU command line.
>>
>> This includes adding a trace event to debug the new time.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>
> Thanks for reworking this -- I think it looks simpler this way.
>
>> diff --git a/include/hw/timer/xlnx-zynqmp-rtc.h 
>> b/include/hw/timer/xlnx-zynqmp-rtc.h
>> index 87649836cc..2867563bdd 100644
>> --- a/include/hw/timer/xlnx-zynqmp-rtc.h
>> +++ b/include/hw/timer/xlnx-zynqmp-rtc.h
>> @@ -79,6 +79,9 @@ typedef struct XlnxZynqMPRTC {
>>      qemu_irq irq_rtc_int;
>>      qemu_irq irq_addr_error_int;
>>
>> +    struct tm current_tm;
>> +    uint32_t tick_offset;
>> +
>
> Do we need current_tm to be a struct field now?
>
>> @@ -143,6 +164,10 @@ static void rtc_reset(DeviceState *dev)
>>          register_reset(&s->regs_info[i]);
>>      }
>>
>> +    trace_xlnx_zynqmp_rtc_gettime(s->current_tm.tm_year, 
>> s->current_tm.tm_mon,
>> +                                  s->current_tm.tm_mday, 
>> s->current_tm.tm_hour,
>> +                                  s->current_tm.tm_min, 
>> s->current_tm.tm_sec);
>> +
>
> ...this trace is in reset, and is the only place we read
> current_tm, except for...
>
>>      rtc_int_update_irq(s);
>>      addr_error_int_update_irq(s);
>>  }
>> @@ -178,14 +203,46 @@ static void rtc_init(Object *obj)
>>      sysbus_init_mmio(sbd, &s->iomem);
>>      sysbus_init_irq(sbd, &s->irq_rtc_int);
>>      sysbus_init_irq(sbd, &s->irq_addr_error_int);
>> +
>> +    qemu_get_timedate(&s->current_tm, 0);
>> +    s->tick_offset = mktimegm(&s->current_tm) -
>> +        qemu_clock_get_ns(rtc_clock) / NANOSECONDS_PER_SECOND;
>
> ...this code in init, which could just use a local variable
> instead, since the trace in reset isn't really reporting
> interesting information any more (the value never changes, so
> you could trace it in init instead).

Yeah you are right. I have moved it all to the init() function.

Alistair

>
>> +}
>>
>
> thanks
> -- PMM