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

[Philippe Mathieu-Daudé]

Hi Luc,

On 4/10/21 3:19 PM, Luc Michel wrote:
> On 08:23 Fri 09 Apr     , Philippe Mathieu-Daudé wrote:
>> 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.
> 
> I think there is a misunderstanding on how the clock API works. If I
> understand correctly your issue, you expect the callback of an input
> clock connected to your constant "main oscillator" clock to be called on
> machine reset.
> 
> If I'm not mistaken this is not the way the API has been designed. The
> callback is called only when the clock period changes. A constant clock
> does not change on reset, so the callback of child clocks should not be
> called.

They why the children of a clock tree fed with constant clock stay with
a clock of 0? Who is responsible of setting their clock to the constant
value?

> However devices that care about this clock value (e.g. a device that
> has a clock input connected to this constant clock) should see their
> standard reset callback called during reset. And they can effectively read
> the clock value here and do what they need to do.
> 
> Note that clock propagation during reset has always been a complicated
> problem. Calling clock_propagate is forbidden during the reset's enter
> phase because of the side effects it can introduce.

Ah... Maybe this is related to the generic reset problem in QEMU :(

>> 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 find your API modification a bit restrictive. I think creating a
> standalone clock can be useful, e.g. in complicated devices that may
> want to use internal "intermediate" clocks. I would not remove this
> possibility to the API users.

Well, this is the point. I can't see a justification to have a clock
on a non-qdev object. We should be able to model complicated devices
with qdev.

We are having various problems with the CPUs which are non-qdev devices,
or recently even with the LED model...:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg798031.html

Phil.