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

[Peter Maydell <peter.maydell@linaro.org>]

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".

-- PMM