Re: [PATCH v4 3/8] Add an internal PLL Clock object

[Arnaud Minier]

Hello Alistair,

Yes, I think we should bail out if pll_set_vco_multiplier receives an invalid 
value to respect the hardware defined bounds.
I actually intended to add a return there but I missed it. It will be added in 
the next version.

Thanks,
Arnaud Minier


----- Mail original -----
De: "Alistair Francis" <alistair23@gmail.com>
À: "Arnaud Minier" <arnaud.minier@telecom-paris.fr>
Cc: "qemu-devel" <qemu-devel@nongnu.org>, "Laurent Vivier" 
<lvivier@redhat.com>, "Alistair Francis" <alistair@alistair23.me>, "Inès 
Varhol" <ines.varhol@telecom-paris.fr>, "Peter Maydell" 
<peter.maydell@linaro.org>, "Paolo Bonzini" <pbonzini@redhat.com>, "Thomas 
Huth" <thuth@redhat.com>, qemu-arm@nongnu.org
Envoyé: Jeudi 1 Février 2024 01:18:22
Objet: Re: [PATCH v4 3/8] Add an internal PLL Clock object

On Wed, Jan 31, 2024 at 2:09 AM Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> This object represents the PLLs and their channels. The PLLs allow for a
> more fine-grained control of the clocks frequency.
>
> 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>
> ---
>  hw/misc/stm32l4x5_rcc.c                   | 175 ++++++++++++++++++++++
>  hw/misc/trace-events                      |   5 +
>  include/hw/misc/stm32l4x5_rcc.h           |  40 +++++
>  include/hw/misc/stm32l4x5_rcc_internals.h |  22 +++
>  4 files changed, 242 insertions(+)
>
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index ed10832f88..fb0233c3e9 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -162,6 +162,156 @@ static void clock_mux_set_source(RccClockMuxState *mux, 
> RccClockMuxSource src)
>      clock_mux_update(mux);
>  }
>
> +static void pll_update(RccPllState *pll)
> +{
> +    uint64_t vco_freq, old_channel_freq, channel_freq;
> +    int i;
> +
> +    /* The common PLLM factor is handled by the PLL mux */
> +    vco_freq = muldiv64(clock_get_hz(pll->in), pll->vco_multiplier, 1);
> +
> +    for (i = 0; i < RCC_NUM_CHANNEL_PLL_OUT; i++) {
> +        if (!pll->channel_exists[i]) {
> +            continue;
> +        }
> +
> +        old_channel_freq = clock_get_hz(pll->channels[i]);
> +        if (!pll->enabled ||
> +            !pll->channel_enabled[i] ||
> +            !pll->channel_divider[i]) {
> +            channel_freq = 0;
> +        } else {
> +            channel_freq = muldiv64(vco_freq,
> +                                    1,
> +                                    pll->channel_divider[i]);
> +        }
> +
> +        /* No change, early continue to avoid log spam and useless 
> propagation */
> +        if (old_channel_freq == channel_freq) {
> +            continue;
> +        }
> +
> +        clock_update_hz(pll->channels[i], channel_freq);
> +        trace_stm32l4x5_rcc_pll_update(pll->id, i, vco_freq,
> +            old_channel_freq, channel_freq);
> +    }
> +}
> +
> +static void pll_src_update(void *opaque, ClockEvent event)
> +{
> +    RccPllState *s = opaque;
> +    pll_update(s);
> +}
> +
> +static void pll_init(Object *obj)
> +{
> +    RccPllState *s = RCC_PLL(obj);
> +    size_t i;
> +
> +    s->in = qdev_init_clock_in(DEVICE(s), "in",
> +                               pll_src_update, s, ClockUpdate);
> +
> +    const char *names[] = {
> +        "out-p", "out-q", "out-r",
> +    };
> +
> +    for (i = 0; i < RCC_NUM_CHANNEL_PLL_OUT; i++) {
> +        s->channels[i] = qdev_init_clock_out(DEVICE(s), names[i]);
> +    }
> +}
> +
> +static void pll_reset_hold(Object *obj)
> +{ }
> +
> +static const VMStateDescription pll_vmstate = {
> +    .name = TYPE_RCC_PLL,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(id, RccPllState),
> +        VMSTATE_CLOCK(in, RccPllState),
> +        VMSTATE_ARRAY_CLOCK(channels, RccPllState,
> +                            RCC_NUM_CHANNEL_PLL_OUT),
> +        VMSTATE_BOOL(enabled, RccPllState),
> +        VMSTATE_UINT32(vco_multiplier, RccPllState),
> +        VMSTATE_BOOL_ARRAY(channel_enabled, RccPllState, 
> RCC_NUM_CHANNEL_PLL_OUT),
> +        VMSTATE_BOOL_ARRAY(channel_exists, RccPllState, 
> RCC_NUM_CHANNEL_PLL_OUT),
> +        VMSTATE_UINT32_ARRAY(channel_divider, RccPllState, 
> RCC_NUM_CHANNEL_PLL_OUT),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void pll_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    rc->phases.hold = pll_reset_hold;
> +    dc->vmsd = &pll_vmstate;
> +}
> +
> +static void pll_set_vco_multiplier(RccPllState *pll, uint32_t vco_multiplier)
> +{
> +    if (pll->vco_multiplier == vco_multiplier) {
> +        return;
> +    }
> +
> +    if (vco_multiplier < 8 || vco_multiplier > 86) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "%s: VCO multiplier is out of bound (%u) for PLL %u\n",
> +            __func__, vco_multiplier, pll->id);

Should we bail out with an invalid value?

Alistair

> +    }
> +
> +    trace_stm32l4x5_rcc_pll_set_vco_multiplier(pll->id,
> +        pll->vco_multiplier, vco_multiplier);
> +
> +    pll->vco_multiplier = vco_multiplier;
> +    pll_update(pll);
> +}
> +
> +static void pll_set_enable(RccPllState *pll, bool enabled)
> +{
> +    if (pll->enabled == enabled) {
> +        return;
> +    }
> +
> +    pll->enabled = enabled;
> +    pll_update(pll);
> +}
> +
> +static void pll_set_channel_enable(RccPllState *pll,
> +                                   PllCommonChannels channel,
> +                                   bool enabled)
> +{
> +    if (pll->channel_enabled[channel] == enabled) {
> +        return;
> +    }
> +
> +    if (enabled) {
> +        trace_stm32l4x5_rcc_pll_channel_enable(pll->id, channel);
> +    } else {
> +        trace_stm32l4x5_rcc_pll_channel_disable(pll->id, channel);
> +    }
> +
> +    pll->channel_enabled[channel] = enabled;
> +    pll_update(pll);
> +}
> +
> +static void pll_set_channel_divider(RccPllState *pll,
> +                                    PllCommonChannels channel,
> +                                    uint32_t divider)
> +{
> +    if (pll->channel_divider[channel] == divider) {
> +        return;
> +    }
> +
> +    trace_stm32l4x5_rcc_pll_set_channel_divider(pll->id,
> +        channel, pll->channel_divider[channel], divider);
> +
> +    pll->channel_divider[channel] = divider;
> +    pll_update(pll);
> +}
> +
>  static void rcc_update_irq(Stm32l4x5RccState *s)
>  {
>      if (s->cifr & CIFR_IRQ_MASK) {
> @@ -465,6 +615,11 @@ static void stm32l4x5_rcc_init(Object *obj)
>
>      qdev_init_clocks(DEVICE(s), stm32l4x5_rcc_clocks);
>
> +    for (i = 0; i < RCC_NUM_PLL; i++) {
> +        object_initialize_child(obj, "pll[*]",
> +                                &s->plls[i], TYPE_RCC_PLL);
> +    }
> +
>      for (i = 0; i < RCC_NUM_CLOCK_MUX; i++) {
>
>          object_initialize_child(obj, "clock[*]",
> @@ -528,6 +683,16 @@ static void stm32l4x5_rcc_realize(DeviceState *dev, 
> Error **errp)
>              return;
>          }
>
> +    for (i = 0; i < RCC_NUM_PLL; i++) {
> +        RccPllState *pll = &s->plls[i];
> +
> +        clock_set_source(pll->in, 
> s->clock_muxes[RCC_CLOCK_MUX_PLL_INPUT].out);
> +
> +        if (!qdev_realize(DEVICE(pll), NULL, errp)) {
> +            return;
> +        }
> +    }
> +
>      for (i = 0; i < RCC_NUM_CLOCK_MUX; i++) {
>          RccClockMuxState *clock_mux = &s->clock_muxes[i];
>
> @@ -548,6 +713,10 @@ static void stm32l4x5_rcc_realize(DeviceState *dev, 
> Error **errp)
>      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);
> +    pll_set_channel_divider(&s->plls[0], 0, 1);
> +    pll_set_enable(&s->plls[0], true);
> +    pll_set_channel_enable(&s->plls[0], 0, true);
> +    pll_set_vco_multiplier(&s->plls[0], 1);
>  }
>
>  static Property stm32l4x5_rcc_properties[] = {
> @@ -585,6 +754,12 @@ static const TypeInfo stm32l4x5_rcc_types[] = {
>          .instance_size = sizeof(RccClockMuxState),
>          .instance_init = clock_mux_init,
>          .class_init = clock_mux_class_init,
> +    }, {
> +        .name = TYPE_RCC_PLL,
> +        .parent = TYPE_DEVICE,
> +        .instance_size = sizeof(RccPllState),
> +        .instance_init = pll_init,
> +        .class_init = pll_class_init,
>      }
>  };
>
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index d5e471811c..1b6054d88a 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -182,6 +182,11 @@ 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 ""
> +stm32l4x5_rcc_pll_set_vco_multiplier(uint32_t pll_id, uint32_t 
> old_multiplier, uint32_t new_multiplier) "RCC: PLL %u: vco_multiplier changed 
> (%u -> %u)"
> +stm32l4x5_rcc_pll_channel_enable(uint32_t pll_id, uint32_t channel_id) "RCC: 
> PLL %u, channel %u enabled"
> +stm32l4x5_rcc_pll_channel_disable(uint32_t pll_id, uint32_t channel_id) 
> "RCC: PLL %u, channel %u disabled"
> +stm32l4x5_rcc_pll_set_channel_divider(uint32_t pll_id, uint32_t channel_id, 
> uint32_t old_divider, uint32_t new_divider) "RCC: PLL %u, channel %u: divider 
> changed (%u -> %u)"
> +stm32l4x5_rcc_pll_update(uint32_t pll_id, uint32_t channel_id, uint64_t 
> vco_freq, uint64_t old_freq, uint64_t new_freq) "RCC: PLL %d channel %d 
> update: vco_freq %" PRIu64 " old_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 6719be9fbe..0fbfba5c40 100644
> --- a/include/hw/misc/stm32l4x5_rcc.h
> +++ b/include/hw/misc/stm32l4x5_rcc.h
> @@ -26,6 +26,15 @@ 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
> +
> +typedef enum PllCommonChannels {
> +    RCC_PLL_COMMON_CHANNEL_P = 0,
> +    RCC_PLL_COMMON_CHANNEL_Q = 1,
> +    RCC_PLL_COMMON_CHANNEL_R = 2,
> +
> +    RCC_NUM_CHANNEL_PLL_OUT = 3
> +} PllCommonChannels;
> +
>  /* 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 */
> @@ -124,6 +133,14 @@ typedef enum RccClockMux {
>      RCC_NUM_CLOCK_MUX
>  } RccClockMux;
>
> +typedef enum RccPll {
> +    RCC_PLL_PLL,
> +    RCC_PLL_PLLSAI1,
> +    RCC_PLL_PLLSAI2,
> +
> +    RCC_NUM_PLL
> +} RccPll;
> +
>  typedef struct RccClockMuxState {
>      DeviceState parent_obj;
>
> @@ -142,6 +159,26 @@ typedef struct RccClockMuxState {
>      struct RccClockMuxState *backref[RCC_NUM_CLOCK_MUX_SRC];
>  } RccClockMuxState;
>
> +typedef struct RccPllState {
> +    DeviceState parent_obj;
> +
> +    RccPll id;
> +    Clock *in;
> +    uint32_t vco_multiplier;
> +    Clock *channels[RCC_NUM_CHANNEL_PLL_OUT];
> +    /* Global pll enabled flag */
> +    bool enabled;
> +    /* 'enabled' refers to the runtime configuration */
> +    bool channel_enabled[RCC_NUM_CHANNEL_PLL_OUT];
> +    /*
> +     * 'exists' refers to the physical configuration
> +     * It should only be set at pll initialization.
> +     * e.g. pllsai2 doesn't have a Q output.
> +     */
> +    bool channel_exists[RCC_NUM_CHANNEL_PLL_OUT];
> +    uint32_t channel_divider[RCC_NUM_CHANNEL_PLL_OUT];
> +} RccPllState;
> +
>  struct Stm32l4x5RccState {
>      SysBusDevice parent_obj;
>
> @@ -187,6 +224,9 @@ struct Stm32l4x5RccState {
>      Clock *sai1_extclk;
>      Clock *sai2_extclk;
>
> +    /* PLLs */
> +    RccPllState plls[RCC_NUM_PLL];
> +
>      /* Muxes ~= outputs */
>      RccClockMuxState clock_muxes[RCC_NUM_CLOCK_MUX];
>
> diff --git a/include/hw/misc/stm32l4x5_rcc_internals.h 
> b/include/hw/misc/stm32l4x5_rcc_internals.h
> index 4aa836848b..a9da5e3be7 100644
> --- a/include/hw/misc/stm32l4x5_rcc_internals.h
> +++ b/include/hw/misc/stm32l4x5_rcc_internals.h
> @@ -22,7 +22,10 @@
>  #include "hw/misc/stm32l4x5_rcc.h"
>
>  #define TYPE_RCC_CLOCK_MUX "stm32l4x5-rcc-clock-mux"
> +#define TYPE_RCC_PLL "stm32l4x5-rcc-pll"
> +
>  OBJECT_DECLARE_SIMPLE_TYPE(RccClockMuxState, RCC_CLOCK_MUX)
> +OBJECT_DECLARE_SIMPLE_TYPE(RccPllState, RCC_PLL)
>
>  /* Register map */
>  REG32(CR, 0x00)
> @@ -285,6 +288,25 @@ REG32(CSR, 0x94)
>                              R_CSR_FWRSTF_MASK   | \
>                              R_CSR_LSIRDY_MASK)
>
> +/* Pll Channels */
> +enum PllChannels {
> +    RCC_PLL_CHANNEL_PLLSAI3CLK = 0,
> +    RCC_PLL_CHANNEL_PLL48M1CLK = 1,
> +    RCC_PLL_CHANNEL_PLLCLK = 2,
> +};
> +
> +enum PllSai1Channels {
> +    RCC_PLLSAI1_CHANNEL_PLLSAI1CLK = 0,
> +    RCC_PLLSAI1_CHANNEL_PLL48M2CLK = 1,
> +    RCC_PLLSAI1_CHANNEL_PLLADC1CLK = 2,
> +};
> +
> +enum PllSai2Channels {
> +    RCC_PLLSAI2_CHANNEL_PLLSAI2CLK = 0,
> +    /* No Q channel */
> +    RCC_PLLSAI2_CHANNEL_PLLADC2CLK = 2,
> +};
> +
>  typedef enum RccClockMuxSource {
>      RCC_CLOCK_MUX_SRC_GND = 0,
>      RCC_CLOCK_MUX_SRC_HSI,
> --
> 2.34.1
>
>