Add write protections for the fields in the CR register.
PLL configuration write protections (among others) have not
been handled yet. This is planned in a future patch set.
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 | 164 ++++++++++++++++++++++++++++------------
1 file changed, 114 insertions(+), 50 deletions(-)
diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
index a3b192e61b..198c6238b6 100644
--- a/hw/misc/stm32l4x5_rcc.c
+++ b/hw/misc/stm32l4x5_rcc.c
@@ -346,9 +346,47 @@ static void rcc_update_irq(Stm32l4x5RccState *s)
}
}
-static void rcc_update_cr_register(Stm32l4x5RccState *s)
+static void rcc_update_msi(Stm32l4x5RccState *s, uint32_t previous_value)
+{
+ uint32_t val;
+
+ static const uint32_t msirange[] = {
+ 100000, 200000, 400000, 800000, 1000000, 2000000,
+ 4000000, 8000000, 16000000, 24000000, 32000000, 48000000
+ };
+ /* MSIRANGE and MSIRGSEL */
+ val = extract32(s->cr, R_CR_MSIRGSEL_SHIFT, R_CR_MSIRGSEL_LENGTH);
+ if (val) {
+ /* MSIRGSEL is set, use the MSIRANGE field */
+ val = extract32(s->cr, R_CR_MSIRANGE_SHIFT, R_CR_MSIRANGE_LENGTH);
+ } else {
+ /* MSIRGSEL is not set, use the MSISRANGE field */
+ val = extract32(s->csr, R_CSR_MSISRANGE_SHIFT, R_CSR_MSISRANGE_LENGTH);
+ }
+
+ if (val < ARRAY_SIZE(msirange)) {
+ clock_update_hz(s->msi_rc, msirange[val]);
+ } else {
+ /*
+ * There is a hardware write protection if the value is out of bound.
+ * Restore the previous value.
+ */
+ s->cr = (s->cr & ~R_CSR_MSISRANGE_MASK) |
+ (previous_value & R_CSR_MSISRANGE_MASK);
+ }
+}
+
+/*
+ * TODO: Add write-protection for all registers:
+ * DONE: CR
+ */
+
+static void rcc_update_cr_register(Stm32l4x5RccState *s, uint32_t previous_value)
{
int val;
+ const RccClockMuxSource current_pll_src =
+ CLOCK_MUX_INIT_INFO[RCC_CLOCK_MUX_PLL_INPUT].src_mapping[
+ s->clock_muxes[RCC_CLOCK_MUX_PLL_INPUT].src];
/* PLLSAI2ON and update PLLSAI2RDY */
val = extract32(s->cr, R_CR_PLLSAI2ON_SHIFT, R_CR_PLLSAI2ON_LENGTH);
@@ -368,77 +406,101 @@ static void rcc_update_cr_register(Stm32l4x5RccState *s)
s->cifr |= R_CIFR_PLLSAI1RDYF_MASK;
}
- /* PLLON and update PLLRDY */
+ /*
+ * PLLON and update PLLRDY
+ * PLLON cannot be reset if the PLL clock is used as the system clock.
+ */
val = extract32(s->cr, R_CR_PLLON_SHIFT, R_CR_PLLON_LENGTH);
- pll_set_enable(&s->plls[RCC_PLL_PLL], val);
- s->cr = (s->cr & ~R_CR_PLLRDY_MASK) |
- (val << R_CR_PLLRDY_SHIFT);
- if (s->cier & R_CIER_PLLRDYIE_MASK) {
- s->cifr |= R_CIFR_PLLRDYF_MASK;
+ if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) != 0b11) {
+ pll_set_enable(&s->plls[RCC_PLL_PLL], val);
+ s->cr = (s->cr & ~R_CR_PLLRDY_MASK) |
+ (val << R_CR_PLLRDY_SHIFT);
+ if (s->cier & R_CIER_PLLRDYIE_MASK) {
+ s->cifr |= R_CIFR_PLLRDYF_MASK;
+ }
+ } else {
+ s->cr |= R_CR_PLLON_MASK;
}
/* CSSON: TODO */
/* HSEBYP: TODO */
- /* HSEON and update HSERDY */
+ /*
+ * HSEON and update HSERDY.
+ * HSEON cannot be reset if the HSE oscillator is used directly or
+ * indirectly as the system clock.
+ */
val = extract32(s->cr, R_CR_HSEON_SHIFT, R_CR_HSEON_LENGTH);
- s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
- (val << R_CR_HSERDY_SHIFT);
- if (val) {
- clock_update_hz(s->hse, s->hse_frequency);
- if (s->cier & R_CIER_HSERDYIE_MASK) {
- s->cifr |= R_CIFR_HSERDYF_MASK;
+ if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) != 0b10 &&
+ current_pll_src != RCC_CLOCK_MUX_SRC_HSE) {
+ s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
+ (val << R_CR_HSERDY_SHIFT);
+ if (val) {
+ clock_update_hz(s->hse, s->hse_frequency);
+ if (s->cier & R_CIER_HSERDYIE_MASK) {
+ s->cifr |= R_CIFR_HSERDYF_MASK;
+ }
+ } else {
+ clock_update_hz(s->hse, 0);
}
} else {
- clock_update_hz(s->hse, 0);
+ s->cr |= R_CR_HSEON_MASK;
}
/* HSIAFS: TODO*/
/* HSIKERON: TODO*/
- /* HSION and update HSIRDY*/
- val = extract32(s->cr, R_CR_HSION_SHIFT, R_CR_HSION_LENGTH);
- s->cr = (s->cr & ~R_CR_HSIRDY_MASK) |
- (val << R_CR_HSIRDY_SHIFT);
- if (val) {
+ /*
+ * HSION and update HSIRDY
+ * HSION is set by hardware if the HSI16 is used directly
+ * or indirectly as system clock.
+ */
+ if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) == 0b01 ||
+ current_pll_src == RCC_CLOCK_MUX_SRC_HSI) {
+ s->cr |= (R_CR_HSION_MASK | R_CR_HSIRDY_MASK);
clock_update_hz(s->hsi16_rc, HSI_FRQ);
if (s->cier & R_CIER_HSIRDYIE_MASK) {
s->cifr |= R_CIFR_HSIRDYF_MASK;
}
} else {
- clock_update_hz(s->hsi16_rc, 0);
+ val = extract32(s->cr, R_CR_HSION_SHIFT, R_CR_HSION_LENGTH);
+ if (val) {
+ clock_update_hz(s->hsi16_rc, HSI_FRQ);
+ s->cr |= R_CR_HSIRDY_MASK;
+ if (s->cier & R_CIER_HSIRDYIE_MASK) {
+ s->cifr |= R_CIFR_HSIRDYF_MASK;
+ }
+ } else {
+ clock_update_hz(s->hsi16_rc, 0);
+ s->cr &= ~R_CR_HSIRDY_MASK;
+ }
}
- static const uint32_t msirange[] = {
- 100000, 200000, 400000, 800000, 1000000, 2000000,
- 4000000, 8000000, 16000000, 24000000, 32000000, 48000000
- };
- /* MSIRANGE and MSIRGSEL */
- val = extract32(s->cr, R_CR_MSIRGSEL_SHIFT, R_CR_MSIRGSEL_LENGTH);
- if (val) {
- /* MSIRGSEL is set, use the MSIRANGE field */
- val = extract32(s->cr, R_CR_MSIRANGE_SHIFT, R_CR_MSIRANGE_LENGTH);
- } else {
- /* MSIRGSEL is not set, use the MSISRANGE field */
- val = extract32(s->csr, R_CSR_MSISRANGE_SHIFT, R_CSR_MSISRANGE_LENGTH);
- }
+ /* MSIPLLEN: TODO */
- if (val < ARRAY_SIZE(msirange)) {
- clock_update_hz(s->msi_rc, msirange[val]);
+ /*
+ * MSION and update MSIRDY
+ * Set by hardware when used directly or indirectly as system clock.
+ */
+ if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) == 0b00 ||
+ current_pll_src == RCC_CLOCK_MUX_SRC_MSI) {
+ s->cr |= (R_CR_MSION_MASK | R_CR_MSIRDY_MASK);
+ if (!(previous_value & R_CR_MSION_MASK) && (s->cier & R_CIER_MSIRDYIE_MASK)) {
+ s->cifr |= R_CIFR_MSIRDYF_MASK;
+ }
+ rcc_update_msi(s, previous_value);
} else {
- clock_update_hz(s->msi_rc, MSI_DEFAULT_FRQ);
- /* TODO: there is a write protection if the value is out of bound,
- implement that instead of setting the default */
- }
-
- /* MSIPLLEN */
-
- /* MSION and update MSIRDY */
- val = extract32(s->cr, R_CR_MSION_SHIFT, R_CR_MSION_LENGTH);
- s->cr = (s->cr & ~R_CR_MSIRDY_MASK) |
- (val << R_CR_MSIRDY_SHIFT);
- if (s->cier & R_CIER_MSIRDYIE_MASK) {
- s->cifr |= R_CIFR_MSIRDYF_MASK;
+ val = extract32(s->cr, R_CR_MSION_SHIFT, R_CR_MSION_LENGTH);
+ if (val) {
+ s->cr |= R_CR_MSIRDY_MASK;
+ rcc_update_msi(s, previous_value);
+ if (s->cier & R_CIER_MSIRDYIE_MASK) {
+ s->cifr |= R_CIFR_MSIRDYF_MASK;
+ }
+ } else {
+ s->cr &= ~R_CR_MSIRDY_MASK;
+ clock_update_hz(s->msi_rc, 0);
+ }
}
rcc_update_irq(s);
}
@@ -963,15 +1025,17 @@ static void stm32l4x5_rcc_write(void *opaque, hwaddr addr,
uint64_t val64, unsigned int size)
{
Stm32l4x5RccState *s = opaque;
+ uint32_t previous_value = 0;
const uint32_t value = val64;
trace_stm32l4x5_rcc_write(addr, value);
switch (addr) {
case A_CR:
+ previous_value = s->cr;
s->cr = (s->cr & CR_READ_SET_MASK) |
(value & (CR_READ_SET_MASK | ~CR_READ_ONLY_MASK));
- rcc_update_cr_register(s);
+ rcc_update_cr_register(s, previous_value);
break;
case A_ICSCR:
s->icscr = value & ~ICSCR_READ_ONLY_MASK;
--
2.34.1On Mon, 19 Feb 2024 at 20:16, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> Add write protections for the fields in the CR register.
> PLL configuration write protections (among others) have not
> been handled yet. This is planned in a future patch set.
Can you make sure you include a suitable prefix (eg
"hw/misc/stm32l4x5_rcc: ") at the front of patch subjects, please?
>
> 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 | 164 ++++++++++++++++++++++++++++------------
> 1 file changed, 114 insertions(+), 50 deletions(-)
>
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index a3b192e61b..198c6238b6 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -346,9 +346,47 @@ static void rcc_update_irq(Stm32l4x5RccState *s)
> }
> }
>
> -static void rcc_update_cr_register(Stm32l4x5RccState *s)
> +static void rcc_update_msi(Stm32l4x5RccState *s, uint32_t previous_value)
> +{
> + uint32_t val;
> +
> + static const uint32_t msirange[] = {
> + 100000, 200000, 400000, 800000, 1000000, 2000000,
> + 4000000, 8000000, 16000000, 24000000, 32000000, 48000000
> + };
> + /* MSIRANGE and MSIRGSEL */
> + val = extract32(s->cr, R_CR_MSIRGSEL_SHIFT, R_CR_MSIRGSEL_LENGTH);
registerfields.h provides macros for "extract a named field", so you
can write this
val = FIELD_EX32(s->cr, CR, MSIRGSEL);
> + if (val) {
> + /* MSIRGSEL is set, use the MSIRANGE field */
> + val = extract32(s->cr, R_CR_MSIRANGE_SHIFT, R_CR_MSIRANGE_LENGTH);
and these as val = extract32(s->cr, CR, MSIRANGE)
and so on.
> + } else {
> + /* MSIRGSEL is not set, use the MSISRANGE field */
> + val = extract32(s->csr, R_CSR_MSISRANGE_SHIFT, R_CSR_MSISRANGE_LENGTH);
> + }
> +
> + if (val < ARRAY_SIZE(msirange)) {
> + clock_update_hz(s->msi_rc, msirange[val]);
> + } else {
> + /*
> + * There is a hardware write protection if the value is out of bound.
> + * Restore the previous value.
> + */
> + s->cr = (s->cr & ~R_CSR_MSISRANGE_MASK) |
> + (previous_value & R_CSR_MSISRANGE_MASK);
> + }
> +}
> +
> - /* HSEON and update HSERDY */
> + /*
> + * HSEON and update HSERDY.
> + * HSEON cannot be reset if the HSE oscillator is used directly or
> + * indirectly as the system clock.
> + */
> val = extract32(s->cr, R_CR_HSEON_SHIFT, R_CR_HSEON_LENGTH);
> - s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
> - (val << R_CR_HSERDY_SHIFT);
> - if (val) {
> - clock_update_hz(s->hse, s->hse_frequency);
> - if (s->cier & R_CIER_HSERDYIE_MASK) {
> - s->cifr |= R_CIFR_HSERDYF_MASK;
> + if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) != 0b10 &&
> + current_pll_src != RCC_CLOCK_MUX_SRC_HSE) {
> + s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
> + (val << R_CR_HSERDY_SHIFT);
> + if (val) {
> + clock_update_hz(s->hse, s->hse_frequency);
> + if (s->cier & R_CIER_HSERDYIE_MASK) {
> + s->cifr |= R_CIFR_HSERDYF_MASK;
> + }
> + } else {
> + clock_update_hz(s->hse, 0);
As I mentioned earlier, please avoid clock_update_hz() for
clock calculations if possible.
thanks
-- PMM
Thank you for the review and for the tips ! It really helps.
I will address the problems you have highlighted and will send a new version later this week.
Arnaud
----- 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>
> Sent: Friday, February 23, 2024 3:59:03 PM
> Subject: Re: [PATCH v5 6/8] Add write protections to CR register
> On Mon, 19 Feb 2024 at 20:16, Arnaud Minier
> <arnaud.minier@telecom-paris.fr> wrote:
>>
>> Add write protections for the fields in the CR register.
>> PLL configuration write protections (among others) have not
>> been handled yet. This is planned in a future patch set.
>
> Can you make sure you include a suitable prefix (eg
> "hw/misc/stm32l4x5_rcc: ") at the front of patch subjects, please?
Sorry. This will be done for the next version.
>
>>
>> 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 | 164 ++++++++++++++++++++++++++++------------
>> 1 file changed, 114 insertions(+), 50 deletions(-)
>>
>> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
>> index a3b192e61b..198c6238b6 100644
>> --- a/hw/misc/stm32l4x5_rcc.c
>> +++ b/hw/misc/stm32l4x5_rcc.c
>> @@ -346,9 +346,47 @@ static void rcc_update_irq(Stm32l4x5RccState *s)
>> }
>> }
>>
>> -static void rcc_update_cr_register(Stm32l4x5RccState *s)
>> +static void rcc_update_msi(Stm32l4x5RccState *s, uint32_t previous_value)
>> +{
>> + uint32_t val;
>> +
>> + static const uint32_t msirange[] = {
>> + 100000, 200000, 400000, 800000, 1000000, 2000000,
>> + 4000000, 8000000, 16000000, 24000000, 32000000, 48000000
>> + };
>> + /* MSIRANGE and MSIRGSEL */
>> + val = extract32(s->cr, R_CR_MSIRGSEL_SHIFT, R_CR_MSIRGSEL_LENGTH);
>
> registerfields.h provides macros for "extract a named field", so you
> can write this
> val = FIELD_EX32(s->cr, CR, MSIRGSEL);
It seems really convenient ! Will use them !
>
>> + if (val) {
>> + /* MSIRGSEL is set, use the MSIRANGE field */
>> + val = extract32(s->cr, R_CR_MSIRANGE_SHIFT, R_CR_MSIRANGE_LENGTH);
>
> and these as val = extract32(s->cr, CR, MSIRANGE)
> and so on.
>
>> + } else {
>> + /* MSIRGSEL is not set, use the MSISRANGE field */
>> + val = extract32(s->csr, R_CSR_MSISRANGE_SHIFT, R_CSR_MSISRANGE_LENGTH);
>> + }
>> +
>> + if (val < ARRAY_SIZE(msirange)) {
>> + clock_update_hz(s->msi_rc, msirange[val]);
>> + } else {
>> + /*
>> + * There is a hardware write protection if the value is out of bound.
>> + * Restore the previous value.
>> + */
>> + s->cr = (s->cr & ~R_CSR_MSISRANGE_MASK) |
>> + (previous_value & R_CSR_MSISRANGE_MASK);
>> + }
>> +}
>> +
>
>> - /* HSEON and update HSERDY */
>> + /*
>> + * HSEON and update HSERDY.
>> + * HSEON cannot be reset if the HSE oscillator is used directly or
>> + * indirectly as the system clock.
>> + */
>> val = extract32(s->cr, R_CR_HSEON_SHIFT, R_CR_HSEON_LENGTH);
>> - s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
>> - (val << R_CR_HSERDY_SHIFT);
>> - if (val) {
>> - clock_update_hz(s->hse, s->hse_frequency);
>> - if (s->cier & R_CIER_HSERDYIE_MASK) {
>> - s->cifr |= R_CIFR_HSERDYF_MASK;
>> + if (extract32(s->cfgr, R_CFGR_SWS_SHIFT, R_CFGR_SWS_LENGTH) != 0b10 &&
>> + current_pll_src != RCC_CLOCK_MUX_SRC_HSE) {
>> + s->cr = (s->cr & ~R_CR_HSERDY_MASK) |
>> + (val << R_CR_HSERDY_SHIFT);
>> + if (val) {
>> + clock_update_hz(s->hse, s->hse_frequency);
>> + if (s->cier & R_CIER_HSERDYIE_MASK) {
>> + s->cifr |= R_CIFR_HSERDYF_MASK;
>> + }
>> + } else {
>> + clock_update_hz(s->hse, 0);
>
> As I mentioned earlier, please avoid clock_update_hz() for
> clock calculations if possible.
This will be changed to use clock_update.
>
> thanks
> -- PMM
© 2016 - 2024 Red Hat, Inc.