Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 4/9/21 4:11 PM, Philippe Mathieu-Daudé wrote:
> On 4/9/21 3:12 PM, Philippe Mathieu-Daudé wrote:
>> On 4/9/21 8:23 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Damian, Luc, Peter.
>>>
>>> I've been debugging some odd issue with the clocks:
>>> a clock created in the machine (IOW, not a qdev clock) isn't
>>> always resetted, thus propagating its value.
>>> "not always" is the odd part. In the MPS2 board, the machine
>>> clock is propagated. Apparently because the peripherals are
>>> created directly in the machine_init() handler. When moving
>>> them out in a SoC QOM container, the clock isn't... I'm still
>>> having hard time to understand what is going on.
>>>
>>> Alternatively I tried to strengthen the clock API by reducing
>>> the clock creation in 2 cases: machine/device. This way clocks
>>> aren't left dangling around alone. The qdev clocks are properly
>>> resetted, and for the machine clocks I register a generic reset
>>> handler. This way is safer, but I don't think we want to keep
>>> adding generic reset handlers, instead we'd like to remove them.
>>>
>>> I'll keep debugging to understand. Meanwhile posting this series
>>> as RFC to get feedback on the approach and start discussing on
>>> this issue.
>>
>> I wonder if this could be the culprit:
> 
> No (same reverting it) :(
> 
>>   commit 96250eab904261b31d9d1ac3abbdb36737635ffa
>>   Author: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>   Date:   Fri Aug 28 10:02:44 2020 +0100
>>
>>       hw/clock: Only propagate clock changes if the clock is changed
>>
>>       Avoid propagating the clock change when the clock does not change.
>>
>>   diff --git a/include/hw/clock.h b/include/hw/clock.h
>>   index d85af45c967..9ecd78b2c30 100644
>>   --- a/include/hw/clock.h
>>   +++ b/include/hw/clock.h
>>   @@ -165,8 +165,9 @@ void clock_propagate(Clock *clk);
>>     */
>>    static inline void clock_update(Clock *clk, uint64_t value)
>>    {
>>   -    clock_set(clk, value);
>>   -    clock_propagate(clk);
>>   +    if (clock_set(clk, value)) {
>>   +        clock_propagate(clk);
>>   +    }
>>    }
>>
>> I.e.:
>>
>> - first use clock_set() to set the new period
>> - then call clock_update() with the same "new period"
>>
>> -> the clock parent already has the new period, so the
>>    children are not updated.
> 
> This is actually what clock_set_source() does:
> 
>   void clock_set_source(Clock *clk, Clock *src)
>   {
>       ...
> 
>       clk->period = src->period; // <------------------------------
>       QLIST_INSERT_HEAD(&src->children, clk, sibling);
>       clk->source = src;
>       clock_propagate_period(clk, false);
>   }
> 
> So indeed if we use qdev_connect_clock_in() in DeviceRealize(),
> it calls clock_set_source() and set the period, does not propagate,
> then later when clock_propagate_period() is called:
> 
> static void clock_propagate_period(Clock *clk, bool call_callbacks)
> {
>     ...
>     QLIST_FOREACH(child, &clk->children, sibling) {
>         if (child->period != clk->period) {
>             //           ^^^^ this condition is false
>             ...
>             clock_propagate_period(child, call_callbacks);
>             // ^^^ children never get clock propagated
>         }
>     }
> }
> 
> Does it make sense?

FWIW this fixes the problem I'm having:

-- >8 --
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 23c0c372b5d..4ecb51b1465 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -87,7 +87,7 @@ static void clock_propagate_period(Clock *clk, bool
call_callbacks)
                                        CLOCK_PERIOD_TO_HZ(clk->period),
                                        CLOCK_PATH(child),
                                        CLOCK_PERIOD_TO_HZ(child->period));
-        if (child->period != clk->period) {
+        if (1) {//child->period != clk->period) {
             if (call_callbacks) {
                 clock_call_callback(child, ClockPreUpdate);
             }
---

At least this confirm my hypothesis and reduces the scope of
the problem.

I'm not sure what is the best way to fix this (yet?) so I'll
wait here for feedback. At least I can keep going :)

Regards,

Phil.