Re: [PATCH] fsl-imx: Add simple RTC emulation for i.MX6 and i.MX7 boards

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 13/12/23 18:20, Peter Maydell wrote:
> On Wed, 13 Dec 2023 at 17:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 12/12/23 15:18, Peter Maydell wrote:
>>> On Sun, 12 Nov 2023 at 09:22, Nikita Ostrenkov <n.ostrenkov@gmail.com> wrote:
>>>
>>> Hi; thanks for this patch, and sorry I haven't got round
>>> to reviewing it earlier.
>>>
>>>> Signed-off-by: Nikita Ostrenkov <n.ostrenkov@gmail.com>
>>>> ---
>>>>    hw/misc/imx7_snvs.c         | 59 ++++++++++++++++++++++++++++++++-----
>>>>    hw/misc/trace-events        |  4 +--
>>>>    include/hw/misc/imx7_snvs.h | 14 ++++++++-
>>>>    3 files changed, 67 insertions(+), 10 deletions(-)
>>
>>
>>>> @@ -31,6 +33,16 @@ struct IMX7SNVSState {
>>>>        SysBusDevice parent_obj;
>>>>
>>>>        MemoryRegion mmio;
>>>> +
>>>> +    /*
>>>> +     * Needed to preserve the tick_count across migration, even if the
>>>> +     * absolute value of the rtc_clock is different on the source and
>>>> +     * destination.
>>>> +     */
>>>> +    int64_t tick_offset_vmstate;
>>>
>>> You don't need tick_offset_vmstate -- it is only in the p031
>>> RTC device as a backwards-compatibility thing for older versions
>>> of QEMU. Migrating tick_offset alone is sufficient in a new
>>> device. (It seems to have been unfortunately copied-and-pasted
>>> into the goldfish RTC device; we should probably fix that bug.)
>>>
>>>> +    int64_t tick_offset;
>>>> +
>>>> +    uint64_t lpcr;
>>>
>>> We've now added state to this device, which means that it needs
>>> a VMState structure to handle migration, and it needs a reset
>>> function.
>>
>> I just noticed your v1 review after reviewing v2. Indeed
>> 'tick_offset' need to be migrated. Now about reset(), RTC
>> are somehow different. When resetting a machine in the same
>> QEMU process I'd expect the RTC offset to not be reset.
>>
>> Could this be clarified by adding a reset handler with no
>> code but a comment /* RTC state is usually kept by CMOS
>> battery and is not reset */ maybe?
> 
> The lpcr register state needs to be reset.

Oh, I missed its access in imx7_snvs_read().

> thanks
> -- PMM