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