Re: [PATCH v4 3/8] Add an internal PLL Clock object
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"
À: "Arnaud Minier"
Cc: "qemu-devel" , "Laurent Vivier"
, "Alistair Francis" , "Inès
Varhol" , "Peter Maydell"
, "Paolo Bonzini" , "Thomas
Huth" , qemu-...@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
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
> Signed-off-by: Inès Varhol
> ---
> 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 = _vmstate;
> +}
> +
> +static void pll_set_vco_multiplier(RccPllState *pll, uint32_
Re: [PATCH v4 3/8] Add an internal PLL Clock object
On Wed, Jan 31, 2024 at 2:09 AM Arnaud Minier
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
> Signed-off-by: Inès Varhol
> ---
> 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 = _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) {
> +
[PATCH v4 3/8] Add an internal PLL Clock object
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
Signed-off-by: Inès Varhol
---
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 = _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);
+}
+
+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) {
