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.

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);
+
+    if (!mux->enabled || !mux->divider) {
+        freq = 0;
+    } else {
+        freq = muldiv64(src_freq, mux->multiplier, mux->divider);
+    }
+
+    /* 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),
+        VMSTATE_BOOL(enabled, RccClockMuxState),
+        VMSTATE_UINT32(src, RccClockMuxState),
+        VMSTATE_UINT32(multiplier, RccClockMuxState),
+        VMSTATE_UINT32(divider, RccClockMuxState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void clock_mux_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    rc->phases.hold = clock_mux_reset_hold;
+    dc->vmsd = &clock_mux_vmstate;
+}
+
+static void clock_mux_set_enable(RccClockMuxState *mux, bool enabled)
+{
+    if (mux->enabled == enabled) {
+        return;
+    }
+
+    if (enabled) {
+        trace_stm32l4x5_rcc_mux_enable(mux->id);
+    } else {
+        trace_stm32l4x5_rcc_mux_disable(mux->id);
+    }
+
+    mux->enabled = enabled;
+    clock_mux_update(mux);
+}
+
+static void clock_mux_set_factor(RccClockMuxState *mux,
+                                 uint32_t multiplier, uint32_t divider)
+{
+    if (mux->multiplier == multiplier && mux->divider == divider) {
+        return;
+    }
+    trace_stm32l4x5_rcc_mux_set_factor(mux->id,
+        mux->multiplier, multiplier, mux->divider, divider);
+
+    mux->multiplier = multiplier;
+    mux->divider = divider;
+    clock_mux_update(mux);
+}
+
+static void clock_mux_set_source(RccClockMuxState *mux, RccClockMuxSource src)
+{
+    if (mux->src == src) {
+        return;
+    }
+
+    trace_stm32l4x5_rcc_mux_set_src(mux->id, mux->src, src);
+    mux->src = src;
+    clock_mux_update(mux);
+}
+
 static void rcc_update_irq(Stm32l4x5RccState *s)
 {
     if (s->cifr & CIFR_IRQ_MASK) {
@@ -329,6 +455,7 @@ static const ClockPortInitArray stm32l4x5_rcc_clocks = {
 static void stm32l4x5_rcc_init(Object *obj)
 {
     Stm32l4x5RccState *s = STM32L4X5_RCC(obj);
+    size_t i;
 
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
 
@@ -338,6 +465,14 @@ static void stm32l4x5_rcc_init(Object *obj)
 
     qdev_init_clocks(DEVICE(s), stm32l4x5_rcc_clocks);
 
+    for (i = 0; i < RCC_NUM_CLOCK_MUX; i++) {
+
+        object_initialize_child(obj, "clock[*]",
+                                &s->clock_muxes[i],
+                                TYPE_RCC_CLOCK_MUX);
+
+    }
+
     s->gnd = clock_new(obj, "gnd");
 }
 
@@ -383,6 +518,7 @@ static const VMStateDescription vmstate_stm32l4x5_rcc = {
 static void stm32l4x5_rcc_realize(DeviceState *dev, Error **errp)
 {
     Stm32l4x5RccState *s = STM32L4X5_RCC(dev);
+    size_t i;
 
     if (s->hse_frequency <  4000000ULL ||
         s->hse_frequency > 48000000ULL) {
@@ -392,10 +528,26 @@ static void stm32l4x5_rcc_realize(DeviceState *dev, Error **errp)
             return;
         }
 
+    for (i = 0; i < RCC_NUM_CLOCK_MUX; i++) {
+        RccClockMuxState *clock_mux = &s->clock_muxes[i];
+
+        if (!qdev_realize(DEVICE(clock_mux), NULL, errp)) {
+            return;
+        }
+    }
+
     clock_update_hz(s->msi_rc, MSI_DEFAULT_FRQ);
     clock_update_hz(s->sai1_extclk, s->sai1_extclk_frequency);
     clock_update_hz(s->sai2_extclk, s->sai2_extclk_frequency);
     clock_update(s->gnd, 0);
+
+    /*
+     * Dummy values to make compilation pass.
+     * Removed in later commits.
+     */
+    clock_mux_set_source(&s->clock_muxes[0], RCC_CLOCK_MUX_SRC_GND);
+    clock_mux_set_enable(&s->clock_muxes[0], true);
+    clock_mux_set_factor(&s->clock_muxes[0], 1, 1);
 }
 
 static Property stm32l4x5_rcc_properties[] = {
@@ -427,6 +579,12 @@ static const TypeInfo stm32l4x5_rcc_types[] = {
         .instance_size  = sizeof(Stm32l4x5RccState),
         .instance_init  = stm32l4x5_rcc_init,
         .class_init     = stm32l4x5_rcc_class_init,
+    }, {
+        .name = TYPE_RCC_CLOCK_MUX,
+        .parent = TYPE_DEVICE,
+        .instance_size = sizeof(RccClockMuxState),
+        .instance_init = clock_mux_init,
+        .class_init = clock_mux_class_init,
     }
 };
 
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 ""
 
 # tz-mpc.c
 tz_mpc_reg_read(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs read: offset 0x%x data 0x%" PRIx64 " size %u"
diff --git a/include/hw/misc/stm32l4x5_rcc.h b/include/hw/misc/stm32l4x5_rcc.h
index 5157e96635..6719be9fbe 100644
--- a/include/hw/misc/stm32l4x5_rcc.h
+++ b/include/hw/misc/stm32l4x5_rcc.h
@@ -26,6 +26,122 @@ OBJECT_DECLARE_SIMPLE_TYPE(Stm32l4x5RccState, STM32L4X5_RCC)
 
 /* In the Stm32l4x5 clock tree, mux have at most 7 sources */
 #define RCC_NUM_CLOCK_MUX_SRC 7
+/* NB: Prescaler are assimilated to mux with one source and one output */
+typedef enum RccClockMux {
+    /* Internal muxes that arent't exposed publicly to other peripherals */
+    RCC_CLOCK_MUX_SYSCLK,
+    RCC_CLOCK_MUX_PLL_INPUT,
+    RCC_CLOCK_MUX_HCLK,
+    RCC_CLOCK_MUX_PCLK1,
+    RCC_CLOCK_MUX_PCLK2,
+    RCC_CLOCK_MUX_HSE_OVER_32,
+    RCC_CLOCK_MUX_LCD_AND_RTC_COMMON,
+
+    /* Muxes with a publicly available output */
+    RCC_CLOCK_MUX_CORTEX_REFCLK,
+    RCC_CLOCK_MUX_USART1,
+    RCC_CLOCK_MUX_USART2,
+    RCC_CLOCK_MUX_USART3,
+    RCC_CLOCK_MUX_UART4,
+    RCC_CLOCK_MUX_UART5,
+    RCC_CLOCK_MUX_LPUART1,
+    RCC_CLOCK_MUX_I2C1,
+    RCC_CLOCK_MUX_I2C2,
+    RCC_CLOCK_MUX_I2C3,
+    RCC_CLOCK_MUX_LPTIM1,
+    RCC_CLOCK_MUX_LPTIM2,
+    RCC_CLOCK_MUX_SWPMI1,
+    RCC_CLOCK_MUX_MCO,
+    RCC_CLOCK_MUX_LSCO,
+    RCC_CLOCK_MUX_DFSDM1,
+    RCC_CLOCK_MUX_ADC,
+    RCC_CLOCK_MUX_CLK48,
+    RCC_CLOCK_MUX_SAI1,
+    RCC_CLOCK_MUX_SAI2,
+
+    /*
+     * Mux that have only one input and one output assigned to as peripheral.
+     * They could be direct lines but it is simpler
+     * to use the same logic for all outputs.
+     */
+    /* - AHB1 */
+    RCC_CLOCK_MUX_TSC,
+    RCC_CLOCK_MUX_CRC,
+    RCC_CLOCK_MUX_FLASH,
+    RCC_CLOCK_MUX_DMA2,
+    RCC_CLOCK_MUX_DMA1,
+
+    /* - AHB2 */
+    RCC_CLOCK_MUX_RNG,
+    RCC_CLOCK_MUX_AES,
+    RCC_CLOCK_MUX_OTGFS,
+    RCC_CLOCK_MUX_GPIOA,
+    RCC_CLOCK_MUX_GPIOB,
+    RCC_CLOCK_MUX_GPIOC,
+    RCC_CLOCK_MUX_GPIOD,
+    RCC_CLOCK_MUX_GPIOE,
+    RCC_CLOCK_MUX_GPIOF,
+    RCC_CLOCK_MUX_GPIOG,
+    RCC_CLOCK_MUX_GPIOH,
+
+    /* - AHB3 */
+    RCC_CLOCK_MUX_QSPI,
+    RCC_CLOCK_MUX_FMC,
+
+    /* - APB1 */
+    RCC_CLOCK_MUX_OPAMP,
+    RCC_CLOCK_MUX_DAC1,
+    RCC_CLOCK_MUX_PWR,
+    RCC_CLOCK_MUX_CAN1,
+    RCC_CLOCK_MUX_SPI3,
+    RCC_CLOCK_MUX_SPI2,
+    RCC_CLOCK_MUX_WWDG,
+    RCC_CLOCK_MUX_LCD,
+    RCC_CLOCK_MUX_TIM7,
+    RCC_CLOCK_MUX_TIM6,
+    RCC_CLOCK_MUX_TIM5,
+    RCC_CLOCK_MUX_TIM4,
+    RCC_CLOCK_MUX_TIM3,
+    RCC_CLOCK_MUX_TIM2,
+
+    /* - APB2 */
+    RCC_CLOCK_MUX_TIM17,
+    RCC_CLOCK_MUX_TIM16,
+    RCC_CLOCK_MUX_TIM15,
+    RCC_CLOCK_MUX_TIM8,
+    RCC_CLOCK_MUX_SPI1,
+    RCC_CLOCK_MUX_TIM1,
+    RCC_CLOCK_MUX_SDMMC1,
+    RCC_CLOCK_MUX_FW,
+    RCC_CLOCK_MUX_SYSCFG,
+
+    /* - BDCR */
+    RCC_CLOCK_MUX_RTC,
+
+    /* - OTHER */
+    RCC_CLOCK_MUX_CORTEX_FCLK,
+
+    RCC_NUM_CLOCK_MUX
+} RccClockMux;
+
+typedef struct RccClockMuxState {
+    DeviceState parent_obj;
+
+    RccClockMux id;
+    Clock *srcs[RCC_NUM_CLOCK_MUX_SRC];
+    Clock *out;
+    bool enabled;
+    uint32_t src;
+    uint32_t multiplier;
+    uint32_t divider;
+
+    /*
+     * Used by clock srcs update callback to retrieve both the clock and the
+     * source number.
+     */
+    struct RccClockMuxState *backref[RCC_NUM_CLOCK_MUX_SRC];
+} RccClockMuxState;
+
 struct Stm32l4x5RccState {
     SysBusDevice parent_obj;
 
@@ -71,6 +187,9 @@ struct Stm32l4x5RccState {
     Clock *sai1_extclk;
     Clock *sai2_extclk;
 
+    /* Muxes ~= outputs */
+    RccClockMuxState clock_muxes[RCC_NUM_CLOCK_MUX];
+
     qemu_irq irq;
     uint64_t hse_frequency;
     uint64_t sai1_extclk_frequency;
diff --git a/include/hw/misc/stm32l4x5_rcc_internals.h b/include/hw/misc/stm32l4x5_rcc_internals.h
index 331ea30db5..4aa836848b 100644
--- a/include/hw/misc/stm32l4x5_rcc_internals.h
+++ b/include/hw/misc/stm32l4x5_rcc_internals.h
@@ -21,6 +21,8 @@
 #include "hw/registerfields.h"
 #include "hw/misc/stm32l4x5_rcc.h"
 
+#define TYPE_RCC_CLOCK_MUX "stm32l4x5-rcc-clock-mux"
+OBJECT_DECLARE_SIMPLE_TYPE(RccClockMuxState, RCC_CLOCK_MUX)
 
 /* Register map */
 REG32(CR, 0x00)
@@ -283,4 +285,31 @@ REG32(CSR, 0x94)
                             R_CSR_FWRSTF_MASK   | \
                             R_CSR_LSIRDY_MASK)
 
+typedef enum RccClockMuxSource {
+    RCC_CLOCK_MUX_SRC_GND = 0,
+    RCC_CLOCK_MUX_SRC_HSI,
+    RCC_CLOCK_MUX_SRC_HSE,
+    RCC_CLOCK_MUX_SRC_MSI,
+    RCC_CLOCK_MUX_SRC_LSI,
+    RCC_CLOCK_MUX_SRC_LSE,
+    RCC_CLOCK_MUX_SRC_SAI1_EXTCLK,
+    RCC_CLOCK_MUX_SRC_SAI2_EXTCLK,
+    RCC_CLOCK_MUX_SRC_PLL,
+    RCC_CLOCK_MUX_SRC_PLLSAI1,
+    RCC_CLOCK_MUX_SRC_PLLSAI2,
+    RCC_CLOCK_MUX_SRC_PLLSAI3,
+    RCC_CLOCK_MUX_SRC_PLL48M1,
+    RCC_CLOCK_MUX_SRC_PLL48M2,
+    RCC_CLOCK_MUX_SRC_PLLADC1,
+    RCC_CLOCK_MUX_SRC_PLLADC2,
+    RCC_CLOCK_MUX_SRC_SYSCLK,
+    RCC_CLOCK_MUX_SRC_HCLK,
+    RCC_CLOCK_MUX_SRC_PCLK1,
+    RCC_CLOCK_MUX_SRC_PCLK2,
+    RCC_CLOCK_MUX_SRC_HSE_OVER_32,
+    RCC_CLOCK_MUX_SRC_LCD_AND_RTC_COMMON,
+
+    RCC_CLOCK_MUX_SRC_NUMBER,
+} RccClockMuxSource;
+
 #endif /* HW_STM32L4X5_RCC_INTERNALS_H */
-- 
2.34.1

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

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