Re: [PATCH for-6.2 09/25] clock: Provide builtin multiplier/divider

[Peter Maydell]

On Tue, 17 Aug 2021 at 10:59, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 8/12/21 11:33 AM, Peter Maydell wrote:

> > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
> > +{
> > +    assert(divider != 0);
> > +
> > +    clk->multiplier = multiplier;
> > +    clk->divider = divider;
> > +}
> > +
> >  static void clock_initfn(Object *obj)
> >  {
> >      Clock *clk = CLOCK(obj);
> >
> > +    clk->multiplier = 1;
> > +    clk->divider = 1;
> > +
> >      QLIST_INIT(&clk->children);
> >  }
> >
> >
>
> Regarding migration, you made the vmstate_muldiv subsection optional. I
> suppose it is to keep backward migration working in case of simple
> mul=1,div=1 clocks.

Yes -- we only send the subsection if the mul,div are
something other than 1,1; an inbound migration stream without
the subsection then means "use 1,1".

> Do we need to ensure multiplier and divider fields are set to 1 upon
> receiving a state if the vmstate_muldiv subsection is absent ? I
> remember there are post_load() tricks to achieve that.

I was relying on "we set the default in clock_initfn()" (by analogy
with being able to assume that fields in device state are at their
reset values when an inbound migration happens, so if the migration
doesn't set them then they stay at those reset values). But
thinking about it a bit more I think you're right and we do have to
have a pre_load function to set them to 1. Otherwise we would get
wrong the case where a board/SoC/device sets a clock up on reset
to have a non-1 multiplier, and then later the guest programs it to
be 1, and then we migrate like that. (That is, the mul/div at point
of migration will be the value-on-reset as set by the device etc
code that created the clock, which might be different from what
clock_initfn() set it to.)

So we need to add something like

static int clock_pre_load(void *opaque)
{
    Clock *clk = opaque;
    /*
     * The initial out-of-reset settings of the Clock might have been
     * configured by the device to be different from what we set
     * in clock_initfn(), so we must here set the default values to
     * be used if they are not in the inbound migration state.
     */
    clk->multiplier = 1;
    clk->divider = 1;
}

thanks
-- PMM