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

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

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?

Also a trace-event "mul: %u -> %u, div: %u -> %u" would be useful IMHO.

> +    clk->multiplier = multiplier;
> +    clk->divider = divider;
> +}