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

[Damien Hedde]

On 8/17/21 12:46 PM, Peter Maydell wrote:
> 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.)

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.hedde@greensocs.com>

Thanks,
--
Damien