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

[Arnaud Minier]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Arnaud Minier" <arnaud.minier@telecom-paris.fr>
> Cc: "qemu-devel" <qemu-devel@nongnu.org>, "Thomas Huth" <thuth@redhat.com>, 
> "Laurent Vivier" <lvivier@redhat.com>, "Inès
> Varhol" <ines.varhol@telecom-paris.fr>, "Samuel Tardieu" 
> <samuel.tardieu@telecom-paris.fr>, "qemu-arm"
> <qemu-arm@nongnu.org>, "Alistair Francis" <alistair@alistair23.me>, "Paolo 
> Bonzini" <pbonzini@redhat.com>, "Alistair
> Francis" <alistair.francis@wdc.com>
> Sent: Friday, February 23, 2024 3:44:59 PM
> Subject: Re: [PATCH v5 2/8] Add an internal clock multiplexer object

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

Thanks for the indication.
I have changed the way we handle the reset to have a three phase one.

> 
> 
>> 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()).

I have changed it to use the period and the builtin clock_set_mul_div() but I 
had to discard
the check below that prevents a lot of spam in the logs due to no longer
having access to the children frequency without using muldiv64 again.
Any idea on how to keep a similar functionnality .

> 
>> +    }
>> +
>> +    /* 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.)

Done. This line was removed.

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

The useless trailing "" have been removed for every commit of this patch set.

> 
> thanks
> -- PMM

Thanks for the review !