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

[Philippe Mathieu-Daudé]

On 8/16/21 11:05 AM, Peter Maydell wrote:
> On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 8/12/21 11:33 AM, Peter Maydell wrote:
>>> It is quite common for a clock tree to involve possibly programmable
>>> clock multipliers or dividers, where the frequency of a clock is for
>>> instance divided by 8 to produce a slower clock to feed to a
>>> particular device.
>>>
>>> Currently we provide no convenient mechanism for modelling this.  You
>>> can implement it by having an input Clock and an output Clock, and
>>> manually setting the period of the output clock in the period-changed
>>> callback of the input clock, but that's quite clunky.
>>>
>>> This patch adds support in the Clock objects themselves for setting a
>>> multiplier or divider.  The effect of setting this on a clock is that
>>> when the clock's period is changed, all the children of the clock are
>>> set to period * multiplier / divider, rather than being set to the
>>> same period as the parent clock.
>>
>> This is super nice!
>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  docs/devel/clocks.rst   | 23 +++++++++++++++++++++++
>>>  include/hw/clock.h      | 29 +++++++++++++++++++++++++++++
>>>  hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++-
>>>  hw/core/clock.c         | 29 +++++++++++++++++++++++++----
>>>  4 files changed, 100 insertions(+), 5 deletions(-)
>>
>>> +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
>>> +{
>>> +    assert(divider != 0);
>>
>> I only wonder if we shouldn't check clock_is_enabled() here.
>> Maybe not assert, but at least report a GUEST_ERROR?
> 
> Setting the multiplier on a disabled clock doesn't seem like
> an error to me. I would expect a common way for the guest to
> program a clock-controller would be "first set the divider
> and any other parameters; finally, enable the clock".

Eh sorry I meant the other way around :/ It is usually either
illegal or undefined behavior on real hw to change a clock scale
while it is active. Personally I'd be interested to catch guests
doing so. I was thinking of:

    if (clock_is_enabled(clk)) {
        qemu_log_mask(LOG_GUEST_ERROR,
                      "Changing scale of ENABLED clock '%s'\n'",
                      CLOCK_PATH(clk));
    }