On 8/17/21 12:46 PM, Peter Maydell wrote:
> On Tue, 17 Aug 2021 at 10:59, Damien Hedde <damien.he...@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.)
Yes, I think we cannot expect the machine init or reset to keep all
clocks in div=1,mul=1 state.
>
> 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;
> }
>
With this callback registered in the main vmsd section,
Reviewed-by: Damien Hedde <damien.he...@greensocs.com>
Thanks,
--
Damien