Re: [PATCH v5 2/8] Add an internal clock multiplexer object

[Peter Maydell]

On Mon, 19 Feb 2024 at 20:12, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> This object is used to represent every multiplexer in the clock tree as
> well as every clock output, every presecaler, frequency multiplier, etc.
> This allows to use a generic approach for every component of the clock tree
> (except the PLLs).
>
> Wasn't sure about how to handle the reset and the migration so used the
> same appproach as the BCM2835 CPRMAN.

I think hw/misc/zynq_sclr.c is also probably a good model to look at.

AIUI the way it works is:
 * input Clock objects must be migrated
 * output Clock objects do not need to be migrated
 * your reset needs to be a three-phase one:
   - in the 'enter' method you reset register values (including
     all the values that define oscillator frequencies, enable bits, etc)
   - in the 'hold' method you compute the values for the output clocks
     as if the input clock is disabled, and propagate them
   - in the 'exit' method you compute the values for the output clocks
     according to the value of the input clock, and propagate them



> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/misc/stm32l4x5_rcc.c                   | 158 ++++++++++++++++++++++
>  hw/misc/trace-events                      |   5 +
>  include/hw/misc/stm32l4x5_rcc.h           | 119 ++++++++++++++++
>  include/hw/misc/stm32l4x5_rcc_internals.h |  29 ++++
>  4 files changed, 311 insertions(+)
>
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index 38ca8aad7d..ed10832f88 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -36,6 +36,132 @@
>  #define LSE_FRQ 32768ULL
>  #define LSI_FRQ 32000ULL
>
> +static void clock_mux_update(RccClockMuxState *mux)
> +{
> +    uint64_t src_freq, old_freq, freq;
> +
> +    src_freq = clock_get_hz(mux->srcs[mux->src]);
> +    old_freq = clock_get_hz(mux->out);

You should try to avoid using clock_get_hz() and clock_update_hz()
when doing clock calculations like this. There is inherently
rounding involved if the clock isn't running at an exact number of Hz.
It's best to use clock_get() and clock_set(), which work with
the clock period specified in units of 2^-32ns.


> +
> +    if (!mux->enabled || !mux->divider) {
> +        freq = 0;
> +    } else {
> +        freq = muldiv64(src_freq, mux->multiplier, mux->divider);

Consider whether you can use the Clock's builtin period
multiplier/divider (clock_set_mul_div()).

> +    }
> +
> +    /* No change, early return to avoid log spam and useless propagation */
> +    if (old_freq == freq) {
> +        return;
> +    }
> +
> +    clock_update_hz(mux->out, freq);
> +    trace_stm32l4x5_rcc_mux_update(mux->id, mux->src, src_freq, freq);
> +}
> +
> +static void clock_mux_src_update(void *opaque, ClockEvent event)
> +{
> +    RccClockMuxState **backref = opaque;
> +    RccClockMuxState *s = *backref;
> +    /*
> +     * The backref value is equal to:
> +     * s->backref + (sizeof(RccClockMuxState *) * update_src).
> +     * By subtracting we can get back the index of the updated clock.
> +     */
> +    const uint32_t update_src = backref - s->backref;
> +    /* Only update if the clock that was updated is the current source*/
> +    if (update_src == s->src) {
> +        clock_mux_update(s);
> +    }
> +}
> +
> +static void clock_mux_init(Object *obj)
> +{
> +    RccClockMuxState *s = RCC_CLOCK_MUX(obj);
> +    size_t i;
> +
> +    for (i = 0; i < RCC_NUM_CLOCK_MUX_SRC; i++) {
> +        char *name = g_strdup_printf("srcs[%zu]", i);
> +        s->backref[i] = s;
> +        s->srcs[i] = qdev_init_clock_in(DEVICE(s), name,
> +                                        clock_mux_src_update,
> +                                        &s->backref[i],
> +                                        ClockUpdate);
> +        g_free(name);
> +    }
> +
> +    s->out = qdev_init_clock_out(DEVICE(s), "out");
> +}
> +
> +static void clock_mux_reset_hold(Object *obj)
> +{ }
> +
> +static const VMStateDescription clock_mux_vmstate = {
> +    .name = TYPE_RCC_CLOCK_MUX,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(id, RccClockMuxState),
> +        VMSTATE_ARRAY_CLOCK(srcs, RccClockMuxState,
> +                            RCC_NUM_CLOCK_MUX_SRC),
> +        VMSTATE_CLOCK(out, RccClockMuxState),

Output clocks don't need VMSTATE_CLOCK lines. (We trust
the device on the other end to migrate its state as needed.)

> +        VMSTATE_BOOL(enabled, RccClockMuxState),
> +        VMSTATE_UINT32(src, RccClockMuxState),
> +        VMSTATE_UINT32(multiplier, RccClockMuxState),
> +        VMSTATE_UINT32(divider, RccClockMuxState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};


> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 62a7599353..d5e471811c 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -177,6 +177,11 @@ stm32l4x5_exti_write(uint64_t addr, uint64_t data) "reg 
> write: addr: 0x%" PRIx64
>  # stm32l4x5_rcc.c
>  stm32l4x5_rcc_read(uint64_t addr, uint32_t data) "RCC: Read <0x%" PRIx64 "> 
> -> 0x%" PRIx32 ""
>  stm32l4x5_rcc_write(uint64_t addr, uint32_t data) "RCC: Write <0x%" PRIx64 
> "> <- 0x%" PRIx32 ""
> +stm32l4x5_rcc_mux_enable(uint32_t mux_id) "RCC: Mux %d enabled"
> +stm32l4x5_rcc_mux_disable(uint32_t mux_id) "RCC: Mux %d disabled"
> +stm32l4x5_rcc_mux_set_factor(uint32_t mux_id, uint32_t old_multiplier, 
> uint32_t new_multiplier, uint32_t old_divider, uint32_t new_divider) "RCC: 
> Mux %d factor changed: multiplier (%u -> %u), divider (%u -> %u)"
> +stm32l4x5_rcc_mux_set_src(uint32_t mux_id, uint32_t old_src, uint32_t 
> new_src) "RCC: Mux %d source changed: from %u to %u"
> +stm32l4x5_rcc_mux_update(uint32_t mux_id, uint32_t src, uint64_t src_freq, 
> uint64_t new_freq) "RCC: Mux %d src %d update: src_freq %" PRIu64 " new_freq 
> %" PRIu64 ""

You don't need the trailing "" in this kind of string
concatenation with a PRIu64 or similar: adding the empty
string on the end of a string has no effect.

thanks
-- PMM