On 8/16/21 11:05 AM, Peter Maydell wrote:
> On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4...@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.mayd...@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));
}
