On Wed, 13 Dec 2023 at 17:17, Philippe Mathieu-Daudé <phi...@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.ostren...@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.ostren...@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.
thanks
-- PMM
