----- Original Message -----
> From: "Peter Maydell" <peter.mayd...@linaro.org>
> To: "Arnaud Minier" <arnaud.min...@telecom-paris.fr>
> Cc: "qemu-devel" <qemu-devel@nongnu.org>, "Thomas Huth" <th...@redhat.com>,
> "Laurent Vivier" <lviv...@redhat.com>, "Inès
> Varhol" <ines.var...@telecom-paris.fr>, "Samuel Tardieu"
> <samuel.tard...@telecom-paris.fr>, "qemu-arm"
> <qemu-...@nongnu.org>, "Alistair Francis" <alist...@alistair23.me>, "Paolo
> Bonzini" <pbonz...@redhat.com>, "Alistair
> Francis" <alistair.fran...@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.min...@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.min...@telecom-paris.fr>
>> Signed-off-by: Inès Varhol <ines.var...@telecom-paris.fr>
>> Acked-by: Alistair Francis <alistair.fran...@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 !
